From 627a71505196b253d5d63fddfabe90a2b1e393bf Mon Sep 17 00:00:00 2001 From: japabu Date: Mon, 28 Sep 2026 21:32:09 +0200 Subject: [PATCH 1/8] USB mass storage: the BOT round trip and the SCSI bring-up are toyos-xhci machines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Bulk-Only round trip and the SCSI above it were the one half of the xHCI driver still hand-written in the kernel's wait module. Every decision in them moves into toyos-xhci as pure code with host tests; kernel/src/drivers/xhci/ wait/msc.rs keeps the transfers, the waits, the phase publishing, the staged actuators and the log lines, so a userland usbd can drive the same machines. toyos_xhci::bot - `cbw`: the 31-byte CBW (BOT 1.0 §5.1), LUN 0, direction from the CDB. - `RoundTrip`: which transfer is owed (CBW, data, CSW), what each completion means, a stalled data phase restarted and its status read (§6.7.2), the one legal status-stall retry (§5.3.3), and `CswDue::judge`, the CSW believed only when valid and meaningful (§6.3), delivered = min(controller, device). - `Broke` with `left` and `event`, generic over the driver's silence reason. A disconnect is its own variant, `Gone`, where it was `Silence { why: Quiet::Gone }`: the crate has to tell it apart and the kernel's `Quiet` is not the crate's. `Broke::left` matches on a `Phase`, not on a phase's name. toyos_xhci::scsi - `Cdb`, built only by this module and carrying its own direction, so no caller passes a length or a direction beside it (the `cdb_len`/`data_in` parameters and the kernel's shape assert are gone). - `Sense` (fourteen bytes or none), `Outcome`, `flushed` (INVALID COMMAND OPERATION CODE is no cache, not a failure), `Transfer` (READ/WRITE(10) batching, and only the first batch may answer "ask again"), and `BringUp`: TEST UNIT READY on a budget, sense, recovery, INQUIRY, READ CAPACITY(10) then (16), and every refusal by name — a stepped machine, driven blocking by the kernel as enumeration's boot scan drives its own. toyos_xhci::ladder::Run holds a device's break count and highest rung; identity::first_language reads descriptor zero's LANGID. No behaviour change on the wire: the same commands, bytes, order and waits. Every log line renders as before, and the two spellings toyos-blackbox holds to the source are kept. What differs off the wire: the CBW is one 31-byte copy where it was field writes through `Unaligned` (same bytes, same offset) and the CSW one 13-byte copy; REQUEST SENSE's response is copied whenever the round trip completed and discarded under 14 bytes; bring-up copies the whole allocation rather than the bytes it reads; a status-stall retry publishes `StatusOwed` twice (the same value); `Transfer::next` panics where the old code truncated a sector number bring-up and the range check already make unreachable; and `msc_flush`'s inner latch check, always true, is gone. sourcegate: the `: u32 = 4096` exception for msc.rs goes with HOST_BLOCK, which is the crate's now. The issue's slug claimed the machine is hand-written in the kernel, which this refutes; what it still owes is the bind, the one scheduling-pass call site, so it is renamed to issues/hardware/a-disk-plugged-in-after-boot-is-bound-inside-a-scheduling-pass.md and both citations move with it. kernel/src: 64984 -> 64697 lines (find kernel/src -name '*.rs' | xargs cat | wc -l); msc.rs 2647 -> 2360. Co-Authored-By: Claude Opus 5.5 --- .../build/the-swarm-is-not-yet-falsifiable.md | 2 +- ...boot-is-bound-inside-a-scheduling-pass.md} | 22 +- ...-small-interrupts-post-and-threads-wait.md | 2 +- kernel/src/drivers/xhci/wait/msc.rs | 879 ++++++------------ src/sourcegate.rs | 5 +- toyos-xhci/src/bot.rs | 541 +++++++++++ toyos-xhci/src/identity.rs | 19 + toyos-xhci/src/ladder.rs | 70 ++ toyos-xhci/src/lib.rs | 1 + toyos-xhci/src/scsi.rs | 859 +++++++++++++++++ 10 files changed, 1799 insertions(+), 601 deletions(-) rename issues/hardware/{the-bot-scsi-machine-is-still-hand-written-in-the-kernel.md => a-disk-plugged-in-after-boot-is-bound-inside-a-scheduling-pass.md} (68%) create mode 100644 toyos-xhci/src/scsi.rs diff --git a/issues/build/the-swarm-is-not-yet-falsifiable.md b/issues/build/the-swarm-is-not-yet-falsifiable.md index 879a39d587..e6872f8c00 100644 --- a/issues/build/the-swarm-is-not-yet-falsifiable.md +++ b/issues/build/the-swarm-is-not-yet-falsifiable.md @@ -207,9 +207,9 @@ issues/build/the-toolchain-ships-no-cargo-and-the-shared-cache-waits-on-one.md issues/build/there-is-no-network-gate.md issues/design-debt/redesign-the-log-subsystem.md issues/diagnostics/the-kernel-keeps-nothing-it-enumerates.md +issues/hardware/a-disk-plugged-in-after-boot-is-bound-inside-a-scheduling-pass.md issues/hardware/a-metal-session-runs-a-pre-flash-gate-first.md issues/hardware/device-shape-and-lifecycle-have-no-coverage.md -issues/hardware/the-bot-scsi-machine-is-still-hand-written-in-the-kernel.md issues/hardware/the-t14-touchpad-is-i2c-hid-and-unbuilt.md issues/hardware/there-is-no-wifi.md issues/isolation/the-power-broker-authority-with-a-human-in-the-loop.md diff --git a/issues/hardware/the-bot-scsi-machine-is-still-hand-written-in-the-kernel.md b/issues/hardware/a-disk-plugged-in-after-boot-is-bound-inside-a-scheduling-pass.md similarity index 68% rename from issues/hardware/the-bot-scsi-machine-is-still-hand-written-in-the-kernel.md rename to issues/hardware/a-disk-plugged-in-after-boot-is-bound-inside-a-scheduling-pass.md index e621b334c4..f1aa91480a 100644 --- a/issues/hardware/the-bot-scsi-machine-is-still-hand-written-in-the-kernel.md +++ b/issues/hardware/a-disk-plugged-in-after-boot-is-bound-inside-a-scheduling-pass.md @@ -4,22 +4,20 @@ kind: track opened: 2026-08-10 --- -# The BOT/SCSI machine is still hand-written in the kernel +# A disk plugged in after boot is bound inside a scheduling pass -The xHCI port, protocol, teardown, recovery and enumeration machines are pure -crate code with a host simulator behind them, and the kernel drives them. The -mass-storage half is not: the BOT round trip and the SCSI bring-up above it are -still hand-written in the kernel's wait module, and that is the one call site +The BOT round trip and the SCSI bring-up above it are pure machines, +`toyos_xhci::bot::RoundTrip` and `toyos_xhci::scsi::BringUp`, and the kernel +drives both blocking, in place. For the read/write entry points that is the +caller's own time. For the bind it is not: `msc::bind` is the one call site where a scheduling pass can still spend its transfer budget inside xHCI — for a disk arriving *after* boot, which is one greppable path. -**What to build**, expressed the way recovery and enumeration already are: the -round trip (command block out, data, status in, one legal stall retry) and the -bring-up above it (test-unit-ready on a budget, sense, inquiry, read-capacity 10 -then 16), with two drivers over the same machine — a blocking one for the -read/write entry points and a stepped one for the bind. Blocked on nothing but -its own size; folding it into the enumeration landing would have made that -unreviewable. +**What to build**: a stepped driver for the bind over the same two machines, +one act per pass, as enumeration has. Moving the bind to a thread that may +block — usbd, step 10 of +`issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md` — ends +this file as well. After it, the pass-duration proof costs no new code: one guest gate measuring a scheduling pass across a plug, plus the existing check-build's pass-cost diff --git a/issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md b/issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md index 614cc26937..0734120d79 100644 --- a/issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md +++ b/issues/kernel/the-kernel-is-small-interrupts-post-and-threads-wait.md @@ -10,7 +10,7 @@ Owner ruling, 2026-09-25: the kernel is to be super small, super performant and safe. This track holds that, and supersedes the ordering of `issues/kernel/every-wait-in-this-kernel-is-a-spin.md`, `issues/kernel/every-driver-is-still-in-the-kernel.md` and -`issues/hardware/the-bot-scsi-machine-is-still-hand-written-in-the-kernel.md`, +`issues/hardware/a-disk-plugged-in-after-boot-is-bound-inside-a-scheduling-pass.md`, which stay as the evidence each stage closes. The design review of 2026-09-25 read the code and found the same shape three diff --git a/kernel/src/drivers/xhci/wait/msc.rs b/kernel/src/drivers/xhci/wait/msc.rs index 8f1fe5421b..99111be478 100644 --- a/kernel/src/drivers/xhci/wait/msc.rs +++ b/kernel/src/drivers/xhci/wait/msc.rs @@ -4,11 +4,13 @@ //! controller: `with_disk` holds the controller lock for the whole of it. //! Everything here comes off the wire and is checked, never trusted; refusal //! is by name, never a panic. +//! +//! **Every decision is the crate's**: the round trip is +//! `toyos_xhci::bot::RoundTrip`, the SCSI above it `toyos_xhci::scsi`, the +//! recovery `toyos_xhci::ladder`. This module does the transfers they ask for, +//! waits for them in place, and says what happened. -//! CBW/CSW use [`crate::mm::Unaligned`] (USB BOT §5.1/§5.2 raw bytes) with no -//! concurrent access to race. - -use crate::mm::{Dma, Unaligned}; +use crate::mm::Dma; use crate::block::{BlockError, BlockResult}; use crate::log; @@ -18,29 +20,29 @@ use super::{Control, Quiet, Restart}; use super::super::{log_unrecoverable, Completion, Disk, StorageGeometry, Trb}; use super::super::{look_for, ports_wanted, with_disk_by, Whereabouts}; use super::super::{TrbRing, XhciController, PAGE, TRB_ADDRESS_DEVICE, TRB_CONFIGURE_EP, TRB_RESET_DEVICE}; -use super::super::{stop, CC_SUCCESS, CC_STALL, CC_SHORT_PACKET, TRB_NORMAL, OFF_INPUT_CTX}; +use super::super::{stop, CC_SUCCESS, TRB_NORMAL, OFF_INPUT_CTX}; use super::super::{AFTER_BREAK, CC_CONTEXT_STATE_ERROR, EP0_DCI}; use super::super::{MSC_IN_RING, MSC_OUT_RING, MSC_CBW, MSC_CSW, MSC_SCRATCH, MSC_SCRATCH_LEN}; use super::super::MSC_INPUT_CTX; use super::super::{MSC_DATA, MSC_DATA_LEN, MSC_MAX_BLOCKS, MSC_STRIDE}; use super::super::device::Endpoint; -use toyos_xhci::bot::Phase; +use toyos_xhci::bot::{self, Bot, Phase, RoundTrip, CBW_LEN, CSW_LEN}; use toyos_xhci::call::{AfterBreak, NotIssued}; use toyos_xhci::configure::{self, BulkEndpoint}; use toyos_xhci::flush::Debt; use toyos_xhci::identity::{self, Identity, Serial, UsbId}; -use toyos_xhci::ladder::{self, AfterReset, Left, PortStep, Rung}; +use toyos_xhci::ladder::{self, AfterReset, Left, PortStep, Run, Rung}; use toyos_xhci::port; use toyos_xhci::reset_recovery::{self, Answered, GaveUp, Look, Pipe, Quiescing, SlotGoes, Step}; +use toyos_xhci::scsi::{self, BringUp, Cdb, Fail, Flushed, Geometry, Heard, Moved, Outcome, Printable}; +use toyos_xhci::scsi::{Refusal, Sense, Transfer, HOST_BLOCK}; /// A region, not an address: the CBW's length is the region's own size, so /// no command can name a length its destination lacks. type DataPhase = Option>; - -/// The block size everything above this driver is written in; a device -/// whose sizes don't divide it is unimplemented, not approximated. -const HOST_BLOCK: u32 = 4096; +/// Why a round trip broke, with this driver's reason for a silence. +type Broke = bot::Broke; /// Wall-clock budget on bring-up's ready attempts: bounds when [`bring_up`] /// stops *starting* attempts, not the one already running. @@ -51,18 +53,8 @@ const READY_BUDGET: Budget = Budget::of( /// The most breaks a device's transport gets in a row before the device is /// offline: one per rung of `toyos_xhci::ladder`. -/// -/// **Per device, not per command**: the run it bounds is the device's, however -/// many callers and operations it is spread over, and only a completed round -/// trip ends it. pub(in crate::drivers::xhci) const MAX_TRANSPORT_BREAKS: u8 = ladder::MOST_BREAKS; -const CBW_SIGNATURE: u32 = 0x4342_5355; -const CSW_SIGNATURE: u32 = 0x5342_5355; -const CBW_LEN: u32 = 31; -const CSW_LEN: u32 = 13; -const TEST_UNIT_READY: [u8; 6] = [0x00; 6]; - /// What the configuration descriptor said about a mass-storage interface; /// both endpoints, always, each valid because `Endpoint` only comes from /// its own constructor. @@ -99,13 +91,9 @@ pub struct MscDevice { in_ring: TrbRing, out_ring: TrbRing, tag: u32, - logical_block_bytes: u32, - sectors_per_block: u32, - blocks: u64, - /// Transport breaks in a row, each answered with one rung of the ladder, - /// and the highest rung among them; a completed round trip clears both. - breaks: u8, - climbed: Option, + /// Zero until bring-up reads the disk's size. + geometry: Geometry, + run: Run, /// Set once the device was taken offline; the device is not spoken to again. failed: bool, /// Where [`XhciController::take_offline`] said the slot goes, until @@ -202,8 +190,8 @@ impl MscDevice { pub fn geometry(&self) -> StorageGeometry { StorageGeometry { - logical_block_bytes: self.logical_block_bytes, - blocks: self.blocks, + logical_block_bytes: self.geometry.sector_bytes, + blocks: self.geometry.blocks, } } @@ -245,16 +233,6 @@ impl MscDevice { } } -/// How one Bulk-Only round trip ended; `delivered` never exceeds the -/// transfer it describes. -enum Bot { - /// CSW status 0; `delivered` is the smaller of what the controller moved - /// and the device says it didn't — else stale data from an earlier LBA leaks. - Done { delivered: u32 }, - /// CSW status 1: the device understood and refused. Sense data says why. - Failed, -} - /// Who a round trip is for, which is whose staging it takes. #[derive(Clone, Copy, PartialEq, Eq)] enum Asks { @@ -275,76 +253,29 @@ pub(in crate::drivers::xhci) struct Enumerated { pub configuration: u8, } -/// Why a Bulk-Only round trip could not be completed; what happened decides -/// which recovery command is legal. -enum Broke { - /// The controller reported this completion code for the named phase, on - /// this pipe. - Code { phase: &'static str, code: u32, pipe: Pipe }, - /// Nothing came back for the named phase, for [`Quiet`]'s reason. - Silence { phase: &'static str, why: Quiet }, - /// The phase moved the wrong byte count; CBW/CSW are fixed length, so - /// short is not a short transfer. - Short { phase: &'static str, moved: u32, wanted: u32 }, - /// The endpoint stalled and the reset did not take. - Stall { phase: &'static str }, - /// CSW status 2: a phase error, which leaves both endpoints Running, so - /// an unconditional Reset Endpoint is illegal here. - PhaseError, - /// The CSW arrived and named somebody else's transfer. The status and - /// residue are the rest of what the device said, and tell a status the - /// device made for an abandoned command from one it made for nothing. - Csw { what: &'static str, got: u32, want: u32, status: u8, residue: u32 }, - /// More bytes claimed unmoved than the transfer had; believing it would - /// underflow the byte count every caller uses. - Residue { unmoved: u32, of: u32 }, -} +/// A break as its line says it. +struct Told<'a>(&'a Broke); -impl Broke { - /// Where the break left the device (`toyos_xhci::ladder::left`); - /// `data_out` is whether the command that broke sends data. - fn left(&self, data_out: bool) -> Left { - let phase = match self { - Self::Code { phase, .. } - | Self::Silence { phase, .. } - | Self::Stall { phase } - | Self::Short { phase, .. } => { - [Phase::Command, Phase::Data].into_iter().find(|p| p.named() == *phase).unwrap_or(Phase::Status) - } - Self::PhaseError | Self::Csw { .. } | Self::Residue { .. } => Phase::Status, - }; - ladder::left(phase, data_out) - } - - /// The transfer event that ended the round trip, where one did: what the - /// quiesce takes ahead of the pipe's Endpoint State field. - fn event(&self) -> Option<(Pipe, u32)> { - match self { - Self::Code { code, pipe, .. } => Some((*pipe, *code)), - _ => None, - } - } -} - -impl core::fmt::Display for Broke { +impl core::fmt::Display for Told<'_> { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - match self { - Self::Code { phase, code, .. } => { + match self.0 { + Broke::Code { phase, code, .. } => { write!(f, "{phase} phase completion {}", Completion(*code)) } - Self::Silence { phase, why } => why.about(phase, "phase", f), - Self::Short { phase, moved, wanted } => { + Broke::Silence { phase, why } => why.about(phase.named(), "phase", f), + Broke::Gone { phase } => Quiet::Gone.about(phase.named(), "phase", f), + Broke::Short { phase, moved, wanted } => { write!(f, "{phase} phase moved {moved} of {wanted} B") } - Self::Stall { phase } => { + Broke::Stall { phase } => { write!(f, "the {phase} phase stalled and the endpoint reset did not clear it") } - Self::PhaseError => f.write_str("the device reported a phase error"), - Self::Csw { what, got, want, status, residue } => write!( + Broke::PhaseError => f.write_str("the device reported a phase error"), + Broke::Csw { what, got, want, status, residue } => write!( f, "CSW {what} {got:#x}, not {want:#x} (status {status}, {residue} B unmoved)" ), - Self::Residue { unmoved, of } => { + Broke::Residue { unmoved, of } => { write!(f, "CSW claims {unmoved} B unmoved of {of}") } } @@ -806,60 +737,42 @@ pub(in crate::drivers::xhci) mod staged { } } -/// The completion of one SCSI command, after the transport's own recovery. -enum Scsi { - Ok { delivered: u32 }, - /// Understood and declined, carrying the sense key/ASC/ASCQ: an optional - /// command's caller must tell "I will not" from "I cannot". - Refused { key: u8, asc: u8, ascq: u8 }, - /// The transport broke, or the device contradicted itself; nothing about - /// the buffer is known. - Broken, - /// Not issued: the caller's [`crate::block::OPERATION`] budget had - /// already expired. Distinct from [`Self::Broken`] because it is not a - /// fact about the disk — [`MscDevice::failed`] stays clear. - Budget, -} - -impl Scsi { - /// SBC's ILLEGAL REQUEST/INVALID COMMAND OPERATION CODE: an answer, not - /// a failure, for a command SBC makes optional. - fn unimplemented(&self) -> bool { - matches!(self, Self::Refused { key: 0x05, asc: 0x20, ascq: 0x00 }) - } - - /// `Ok` never reaches here — each of the three callers has its own idea - /// of what a complete transfer is. - fn as_block_error(&self) -> BlockError { - match self { - Self::Budget => BlockError::BudgetExpired, - _ => BlockError::Device, - } - } -} - /// The one line a device's refusal produces, wherever it is noticed — one /// function so per-caller wording never obscures what the device said. -fn log_refusal(cdb: &[u8], key: u8, asc: u8, ascq: u8) { - log!( - "usb-storage: SCSI {:#04x} failed, sense {key:#04x}/{asc:#04x}/{ascq:#04x}", - cdb.first().copied().unwrap_or(0) - ); +fn log_refusal(cdb: &Cdb, sense: Sense) { + log!("usb-storage: SCSI {:#04x} failed, sense {sense}", cdb.opcode()); } /// The sense a test actuator makes SYNCHRONIZE CACHE answer with, or `None` /// on a shipped kernel. ILLEGAL REQUEST/INVALID COMMAND OPERATION CODE must /// not fail the caller; HARDWARE ERROR/INTERNAL TARGET FAILURE must. -fn flush_sense() -> Option<(u8, u8, u8)> { +fn flush_sense() -> Option { if crate::actuator::usb_flush_unimplemented() { - Some((0x05, 0x20, 0x00)) + Some(Sense { key: 0x05, asc: 0x20, ascq: 0x00 }) } else if crate::actuator::usb_flush_fails() { - Some((0x04, 0x44, 0x00)) + Some(Sense { key: 0x04, asc: 0x44, ascq: 0x00 }) } else { None } } +/// A bulk transfer's completion, as the round trip hears it. +fn completed(completion: Result<(u32, u32), Quiet>) -> bot::Answer { + match completion { + Ok((code, residue)) => bot::Answer::Moved { code, residue }, + Err(Quiet::Gone) => bot::Answer::Gone, + Err(why) => bot::Answer::Silent(why), + } +} + +/// What a caller above the disk is told. +fn block_error(fail: Fail) -> BlockError { + match fail { + Fail::Device => BlockError::Device, + Fail::Budget => BlockError::BudgetExpired, + } +} + /// Snapshot of `dev.block`'s address and value at the top of one round /// trip, to catch a write to `MscDevice` from outside the driver while a /// phase is waiting. @@ -968,38 +881,29 @@ impl XhciController { if dev.no_write_cache { return Ok(()); } - // LBA 0, count 0: the whole medium — all a block-device flush can mean. - let cdb = [0x35u8, 0, 0, 0, 0, 0, 0, 0, 0, 0]; - let issued = ctrl.scsi(dev, &cdb, 10, None, false, until); - let outcome = match flush_sense() { - Some((key, asc, ascq)) => Scsi::Refused { key, asc, ascq }, - None => issued, - }; - // No write cache means nothing here could have been made durable, - // so reporting a failure would report the wrong thing. - if outcome.unimplemented() { - if !dev.no_write_cache { + let cdb = Cdb::SYNCHRONIZE_CACHE; + let issued = ctrl.scsi(dev, &cdb, None, until); + let outcome = flush_sense().map_or(issued, Outcome::Refused); + match scsi::flushed(outcome) { + Flushed::NoCache => { dev.no_write_cache = true; log!("usb-storage: disk {number} does not implement SYNCHRONIZE CACHE \ (sense 0x05/0x20/0x00); its writes are durable once they complete"); + Ok(()) } - return Ok(()); - } - match outcome { - Scsi::Ok { .. } => { + Flushed::Emptied => { dev.debt.flushed(); #[cfg(feature = "boot-actuators")] transport_break::flushed(); Ok(()) } - Scsi::Refused { key, asc, ascq } => { - log_refusal(&cdb, key, asc, ascq); + Flushed::Refused(sense) => { + log_refusal(&cdb, sense); Err(BlockError::Device) } - Scsi::Broken => Err(BlockError::Device), - // Unlogged: `scsi` already named the budget, and a line here + // Unlogged: `scsi` already named a budget, and a line here // would itself be the next flush. - Scsi::Budget => Err(BlockError::BudgetExpired), + Flushed::Ended(fail) => Err(block_error(fail)), } }) .unwrap_or(Err(BlockError::Device)) @@ -1022,72 +926,41 @@ impl XhciController { if dev.failed { return Err(BlockError::Device); } - if count == 0 { - return Ok(()); - } - match lba.checked_add(count as u64) { - Some(end) if end <= dev.blocks => {} - _ => { - log!("usb-storage: {lba}+{count} is past the {} blocks this disk has", dev.blocks); + let mut transfer = match Transfer::new(lba, count, write, &dev.geometry, MSC_MAX_BLOCKS) { + Ok(transfer) => transfer, + Err(past) => { + log!("usb-storage: {lba}+{count} is past the {} blocks this disk has", past.blocks); return Err(BlockError::Device); } - } + }; let dma = self.dma(); let data = dma.subview(dev.block + MSC_DATA, MSC_DATA_LEN); - let mut done = 0u32; - while done < count { - let batch = (count - done).min(MSC_MAX_BLOCKS); - let bytes = batch as usize * HOST_BLOCK as usize; - let offset = done as usize * HOST_BLOCK as usize; - let sector_lba = (lba + done as u64) * dev.sectors_per_block as u64; - let sectors = batch * dev.sectors_per_block; - - // `bring_up` refused any disk whose last sector doesn't fit 32 - // bits, so READ/WRITE(10) can address every block reported. - let lba32 = sector_lba as u32; - let cdb = [ - if write { 0x2Au8 } else { 0x28 }, - 0, - (lba32 >> 24) as u8, - (lba32 >> 16) as u8, - (lba32 >> 8) as u8, - lba32 as u8, - 0, - (sectors >> 8) as u8, - sectors as u8, - 0, - ]; - + while let Some(batch) = transfer.next() { + let (bytes, offset) = (batch.bytes, batch.offset); if let Host::From(src) = &host { dma.copy_from(dev.block + MSC_DATA, &src[offset..offset + bytes]); } - - match self.scsi(dev, &cdb, 10, Some(data.subview(0, bytes)), !write, until) { - Scsi::Ok { delivered } if delivered as usize == bytes => dev.wrote(write), - // Short of what was asked: nothing above can say which - // blocks arrived, so a partial transfer is a failed one. - Scsi::Ok { delivered } => { - dev.wrote(write); - log!("usb-storage: {delivered} of {bytes} B at block {}", lba + done as u64); + let outcome = self.scsi(dev, &batch.cdb, Some(data.subview(0, bytes)), until); + let moved = transfer.answered(&batch, outcome); + if moved.reported() { + dev.wrote(write); + } + match moved { + Moved::Whole => {} + Moved::Short { delivered } => { + log!("usb-storage: {delivered} of {bytes} B at block {}", batch.block); return Err(BlockError::Device); } - Scsi::Refused { key, asc, ascq } => { - log_refusal(&cdb, key, asc, ascq); + Moved::Refused(sense) => { + log_refusal(&batch.cdb, sense); return Err(BlockError::Device); } - // `done > 0` means blocks already moved are on the device - // with no way to resume; only the first batch may answer "ask - // again". - other @ (Scsi::Broken | Scsi::Budget) => { - return Err(if done == 0 { other.as_block_error() } else { BlockError::Device }); - } + Moved::Ended(fail) => return Err(block_error(fail)), } - if let Host::Into(dst) = &mut host { dma.copy_to(dev.block + MSC_DATA, &mut dst[offset..offset + bytes]); } - done += batch; } Ok(()) } @@ -1111,18 +984,9 @@ impl XhciController { /// by whoever the call is — [`served`] for a block operation, the bind for /// each of its commands — so a later command of the same operation spends /// what the break left it, and no call inherits another's. - #[allow(clippy::too_many_arguments)] - fn scsi( - &mut self, - dev: &mut MscDevice, - cdb: &[u8], - cdb_len: u8, - data: DataPhase, - data_in: bool, - until: Deadline, - ) -> Scsi { - let opcode = cdb.first().copied().unwrap_or(0); - let data_out = data.is_some() && !data_in; + fn scsi(&mut self, dev: &mut MscDevice, cdb: &Cdb, data: DataPhase, until: Deadline) -> Outcome { + let opcode = cdb.opcode(); + let data_out = data.is_some() && !cdb.data_in(); // Named per line so a multi-disk boot's retry log attributes to the // right disk. let slot = self.slot(dev.slot_id); @@ -1136,31 +1000,31 @@ impl XhciController { Err(NotIssued::Operation) => { log!("usb-storage: {slot} SCSI {opcode:#04x} not issued: {}", crate::block::OPERATION); - return Scsi::Budget; + return Outcome::Budget; } // A command re-issued with nothing left would have every wait // cut at once, and count against the device a break that was // the budget's. Err(NotIssued::Call(why)) => { log!("usb-storage: {slot} SCSI {opcode:#04x} not issued again: {why}"); - return Scsi::Budget; + return Outcome::Budget; } } - match self.bot(dev, cdb, cdb_len, data, data_in, Asks::Command) { + match self.bot(dev, cdb, data, Asks::Command) { Ok(Bot::Done { delivered }) => { self.transport_came_back(dev, opcode); - return Scsi::Ok { delivered }; + return Outcome::Ok { delivered }; } Ok(Bot::Failed) => { self.transport_came_back(dev, opcode); - let (key, asc, ascq) = self.request_sense(dev); - return Scsi::Refused { key, asc, ascq }; + return Outcome::Refused(self.request_sense(dev)); } // Not a transport that broke: a device that is no longer on // the bus. Its port's own teardown gives the slot and the pool // block back, and a recovery or a reset aimed at an empty port // would only spend their bounds. - Err(broke @ Broke::Silence { why: Quiet::Gone, .. }) => { + Err(gone @ Broke::Gone { .. }) => { + let broke = Told(&gone); log!("usb-storage: {slot} transport broke on SCSI {opcode:#04x}: {broke}; \ its port's teardown takes it from here"); dev.failed = true; @@ -1168,14 +1032,15 @@ impl XhciController { // A hold for its device is part of this call, from the // wait that saw it go. self.after_break.open(self.bulk_began, AFTER_BREAK); - return Scsi::Broken; + return Outcome::Broken; } - Err(broke) => { + Err(why) => { self.after_break.open(self.bulk_began, AFTER_BREAK); + let broke = Told(&why); log!("usb-storage: {slot} transport broke on SCSI {opcode:#04x}: {broke}; \ - break {} of {MAX_TRANSPORT_BREAKS} running", dev.breaks.saturating_add(1)); - if !self.climb_until_in_step(dev, broke.event(), broke.left(data_out)) { - return Scsi::Broken; + break {} of {MAX_TRANSPORT_BREAKS} running", dev.run.breaks().saturating_add(1)); + if !self.climb_until_in_step(dev, why.event(), why.left(data_out)) { + return Outcome::Broken; } } } @@ -1213,21 +1078,19 @@ impl XhciController { fn climb(&mut self, dev: &mut MscDevice, mut broke: Option<(Pipe, u32)>, mut left: Left) -> bool { let slot = self.slot(dev.slot_id); loop { - dev.breaks = dev.breaks.saturating_add(1); - let rung = ladder::next(dev.climbed, left); - if dev.climbed.is_none() && rung != Rung::ClassReset { + let ladder::Climb { rung, skips_class_reset } = dev.run.broke(left); + if skips_class_reset { log!("usb-storage: {slot} is owed the data of the command that broke, so nothing \ can be asked of it on the Bulk-Out: its port is reset with no class reset \ before it"); } - dev.climbed = Some(rung); self.after_break.enter(rung, crate::clock::nanos_since_boot()); let climbed = match rung { Rung::ClassReset => self.reset_recovery(dev, broke), Rung::PortReset => self.port_reset_recovery(dev, broke), Rung::Offline => { log!("usb-storage: {slot} broke {} times running; its port reset did not \ - bring the transport back", dev.breaks); + bring the transport back", dev.run.breaks()); self.take_offline(dev, broke); return false; } @@ -1238,9 +1101,9 @@ impl XhciController { return true; } Climbed::OutOfStep(why) => { - log!("usb-storage: {slot} transport broke on the {}'s TEST UNIT READY: {why}; \ + log!("usb-storage: {slot} transport broke on the {}'s TEST UNIT READY: {}; \ break {} of {MAX_TRANSPORT_BREAKS} running", - rung.named(), dev.breaks.saturating_add(1)); + rung.named(), Told(&why), dev.run.breaks().saturating_add(1)); broke = why.event(); } // As a round trip whose port read disconnected mid-wait: a @@ -1252,7 +1115,7 @@ impl XhciController { } Climbed::Failed => { log!("usb-storage: {slot} the {} was not answered; break {} of \ - {MAX_TRANSPORT_BREAKS} running", rung.named(), dev.breaks.saturating_add(1)); + {MAX_TRANSPORT_BREAKS} running", rung.named(), dev.run.breaks().saturating_add(1)); // The event is spent: the rung has commanded the pair // since, and only the fields speak for it now. broke = None; @@ -1266,8 +1129,7 @@ impl XhciController { /// A round trip completed: the run of breaks is over, and the log says so /// where there was one. fn transport_came_back(&mut self, dev: &mut MscDevice, opcode: u8) { - let breaks = core::mem::take(&mut dev.breaks); - dev.climbed = None; + let breaks = dev.run.over(); if breaks > 0 { log!("usb-storage: {} SCSI {opcode:#04x} completed after {breaks} break(s) running; \ the transport came back and the count is cleared", self.slot(dev.slot_id)); @@ -1445,7 +1307,7 @@ impl XhciController { return Climbed::Failed; } } - match self.bot(dev, &TEST_UNIT_READY, 6, None, false, Asks::Verification(Rung::PortReset)) { + match self.bot(dev, &Cdb::TEST_UNIT_READY, None, Asks::Verification(Rung::PortReset)) { Ok(answer) => { log!("usb-storage: {slot} the port reset took: addressed and configured again, the \ device answered TEST UNIT READY under its own tag {:#x}", dev.tag); @@ -1460,72 +1322,33 @@ impl XhciController { /// whoever asks next, which would otherwise be the command the rung is for. fn take_held_sense(&mut self, dev: &mut MscDevice, answer: Bot) { if matches!(answer, Bot::Failed) { - let (key, asc, ascq) = self.request_sense(dev); - log!("usb-storage: {} held sense {key:#04x}/{asc:#04x}/{ascq:#04x} after its recovery", - self.slot(dev.slot_id)); + let sense = self.request_sense(dev); + log!("usb-storage: {} held sense {sense} after its recovery", self.slot(dev.slot_id)); } } - /// REQUEST SENSE as (key, ASC, ASCQ), zeroed if the device would not - /// say — zero is the failing side of every decision made from it. - fn request_sense(&mut self, dev: &mut MscDevice) -> (u8, u8, u8) { + /// REQUEST SENSE through `bot` directly, so it cannot recurse into asking + /// for sense about itself. + fn request_sense(&mut self, dev: &mut MscDevice) -> Sense { let dma = self.dma(); let scratch = dma.subview(dev.block + MSC_SCRATCH, MSC_SCRATCH_LEN); scratch.zero(); - let cdb = [0x03u8, 0, 0, 0, 18, 0]; - // Goes through `bot` directly, so it cannot recurse into asking for - // sense about itself. ASCQ is byte 13, so 14 bytes must arrive or all - // three stay zero, which is what `Scsi::unimplemented` tests for. - match self.bot(dev, &cdb, 6, Some(scratch.subview(0, 18)), true, Asks::Command) { - Ok(Bot::Done { delivered }) if delivered >= 14 => { - let mut resp = [0u8; 18]; - dma.copy_to(dev.block + MSC_SCRATCH, &mut resp); - (resp[2] & 0x0F, resp[12], resp[13]) + let region = scratch.subview(0, scsi::SENSE_BYTES); + match self.bot(dev, &Cdb::REQUEST_SENSE, Some(region), Asks::Command) { + Ok(Bot::Done { delivered }) => { + let mut response = [0u8; scsi::SENSE_BYTES]; + dma.copy_to(dev.block + MSC_SCRATCH, &mut response); + Sense::of(&response, delivered) } - _ => (0, 0, 0), + _ => Sense::NONE, } } - /// One fixed-length leg of the round trip (command or status block), - /// which the device must take or give in full. - fn framed_phase( - &mut self, - dev: &mut MscDevice, - in_dir: bool, - phys: u64, - len: u32, - phase: Phase, - open: &stop::OpenCommand, - ) -> Result<(), Broke> { - let what = phase.named(); - match self.bulk(dev, in_dir, phys, len, phase, open) { - // Short Packet is how the xHC reports a sub-maximum-packet - // transfer (a 13-byte CSW on a 512-byte endpoint); zero residue - // means it all arrived. - Ok((CC_SUCCESS | CC_SHORT_PACKET, 0)) => Ok(()), - Ok((CC_SUCCESS | CC_SHORT_PACKET, residue)) => Err(Broke::Short { - phase: what, - moved: len.saturating_sub(residue), - wanted: len, - }), - Ok((code, _)) => Err(Broke::Code { phase: what, code, pipe: Pipe::of(in_dir) }), - Err(why) => Err(Broke::Silence { phase: what, why }), - } - } - - /// The Bulk-Only Transport round trip: command block out, data, status in. - fn bot( - &mut self, - dev: &mut MscDevice, - cdb: &[u8], - cdb_len: u8, - data: DataPhase, - data_in: bool, - asks: Asks, - ) -> Result { - // The CDBs are this file's own, so their shape is a kernel invariant. - assert!(cdb_len as usize <= cdb.len() && cdb_len <= 16); + /// One Bulk-Only round trip, as `toyos_xhci::bot::RoundTrip` asks for it: + /// each transfer queued and waited for in place. + fn bot(&mut self, dev: &mut MscDevice, cdb: &Cdb, data: DataPhase, asks: Asks) -> Result { crate::block::census::command_issued(); + let data_in = cdb.data_in(); // The length the device is told to move is the region's own, so the // only bound left to state is this driver's largest transfer. let (data_phys, data_len) = match data { @@ -1543,13 +1366,13 @@ impl XhciController { #[cfg(feature = "boot-actuators")] let staged = match asks { Asks::Verification(rung) => staged::take_probe(rung), - Asks::Command => staged::take(cdb.first().copied().unwrap_or(0)), + Asks::Command => staged::take(cdb.opcode()), }; #[cfg(not(feature = "boot-actuators"))] let _ = asks; #[cfg(feature = "boot-actuators")] if staged == Some(staged::Fault::PortGone) { - return Err(Broke::Silence { phase: "command", why: Quiet::Gone }); + return Err(Broke::Gone { phase: Phase::Command }); } #[cfg(feature = "boot-actuators")] if staged == Some(staged::Fault::Unanswered) { @@ -1560,27 +1383,17 @@ impl XhciController { let now = crate::clock::nanos_since_boot(); let cut = self.after_break.cut(began, now, super::super::USB_TIMEOUT_NS); let why = if cut { Quiet::Spent } else { Quiet::Elapsed }; - return Err(Broke::Silence { phase: "status", why }); + return Err(Broke::Silence { phase: Phase::Status, why }); } let dma = self.dma(); let tag = dev.next_tag(); #[cfg(feature = "stack-witness")] let entered_with = block_witness(dev); - // Unaligned per the file header; bounded by CBW_LEN (15+cdb_len <= - // 31) and exclusive — not yet enqueued. - let cbw: Dma<'static, Unaligned> = - super::super::zero_dma(dma, dev.block + MSC_CBW, CBW_LEN as usize).unaligned(); - cbw.write::(0, CBW_SIGNATURE.to_le()); - cbw.write::(4, tag.to_le()); - cbw.write::(8, data_len.to_le()); - cbw.write::(12, if data_in { 0x80 } else { 0x00 }); - cbw.write::(13, 0); // LUN 0: this driver binds one logical unit - cbw.write::(14, cdb_len); - cbw.copy_from(15, &cdb[..cdb_len as usize]); + dma.copy_from(dev.block + MSC_CBW, &bot::cbw(tag, data_len, cdb)); #[cfg(feature = "boot-actuators")] if staged == Some(staged::Fault::BadSignature) { - cbw.write::(0, 0); + dma.copy_from(dev.block + MSC_CBW, &[0; 4]); } // **From here the device is one this kernel has spoken a command to**, @@ -1597,119 +1410,75 @@ impl XhciController { ctx_size: self.context_size as u32, data_in, }); - - let cbw_phys = dma.device_addr() + (dev.block + MSC_CBW) as u64; #[cfg(feature = "boot-actuators")] - let withheld = staged == Some(staged::Fault::NoCbw); - #[cfg(not(feature = "boot-actuators"))] - let withheld = false; - if !withheld { - self.framed_phase(dev, false, cbw_phys, CBW_LEN, Phase::Command, &open)?; - } + let write = cdb.opcode() == scsi::WRITE_10; - // What the controller says reached the buffer; checked against the - // CSW's residue below. - let mut moved = 0u32; - if data_len > 0 { - // The gap the class leaves open: the device has the CBW and this - // kernel has queued nothing for it. - open.at(Phase::DataOwed, &dev.in_ring, &dev.out_ring); - #[cfg(feature = "boot-actuators")] - if cdb.first() == Some(&0x2A) { - transport_break::arm(dev.owes_a_flush()); - mid_write::wedge_if_staged(Phase::DataOwed); - } - #[cfg(feature = "boot-actuators")] - let held = short_read::hold( - dma, - (data_phys - dma.device_addr()) as usize, - data_len, - data_in && cdb.first() == Some(&0x28), - ); - let completion = self.bulk(dev, data_in, data_phys, data_len, Phase::Data, &open); - #[cfg(feature = "boot-actuators")] - let completion = short_read::release(dma, held, completion); - match completion { - Ok((CC_SUCCESS | CC_SHORT_PACKET, unmoved)) => { - moved = data_len.saturating_sub(unmoved); + let (mut trip, mut act) = RoundTrip::begin(tag, data_len, cdb); + loop { + let answer = match act { + bot::Act::Command => { + #[cfg(feature = "boot-actuators")] + let withheld = staged == Some(staged::Fault::NoCbw); + #[cfg(not(feature = "boot-actuators"))] + let withheld = false; + if withheld { + bot::Answer::Moved { code: CC_SUCCESS, residue: 0 } + } else { + let cbw_phys = dma.device_addr() + (dev.block + MSC_CBW) as u64; + completed(self.bulk(dev, false, cbw_phys, CBW_LEN as u32, Phase::Command, &open)) + } } - // A stalled data phase is ordinary (unsupported command, - // read past the end); recovering it and reading the status - // turns it into a clean refusal. - Ok((CC_STALL, unmoved)) => { - if !self.restart_bulk(dev, data_in) { - return Err(Broke::Stall { phase: "data" }); + bot::Act::Data(pipe) => { + open.at(Phase::DataOwed, &dev.in_ring, &dev.out_ring); + #[cfg(feature = "boot-actuators")] + if write { + transport_break::arm(dev.owes_a_flush()); + mid_write::wedge_if_staged(Phase::DataOwed); } - // The recovery rebuilt the ring this phase was on, so the - // point the account reads is republished before anything - // else reaches the controller. - open.at(Phase::Data, &dev.in_ring, &dev.out_ring); - moved = data_len.saturating_sub(unmoved); + #[cfg(feature = "boot-actuators")] + let held = short_read::hold( + dma, + (data_phys - dma.device_addr()) as usize, + data_len, + data_in && cdb.opcode() == scsi::READ_10, + ); + let completion = self.bulk(dev, pipe == Pipe::In, data_phys, data_len, Phase::Data, &open); + #[cfg(feature = "boot-actuators")] + let completion = short_read::release(dma, held, completion); + completed(completion) } - Ok((code, _)) => { - return Err(Broke::Code { phase: "data", code, pipe: Pipe::of(data_in) }) + bot::Act::Status => { + open.at(Phase::StatusOwed, &dev.in_ring, &dev.out_ring); + #[cfg(feature = "boot-actuators")] + if data_len > 0 && write { + mid_write::wedge_if_staged(Phase::StatusOwed); + } + super::super::zero_dma(dma, dev.block + MSC_CSW, CSW_LEN); + let csw_phys = dma.device_addr() + (dev.block + MSC_CSW) as u64; + completed(self.bulk(dev, true, csw_phys, CSW_LEN as u32, Phase::Status, &open)) } - Err(why) => return Err(Broke::Silence { phase: "data", why }), - } - } - - // The second gap: the data phase is done, or there was none, and the - // device is holding a CSW nothing has asked for. - open.at(Phase::StatusOwed, &dev.in_ring, &dev.out_ring); - #[cfg(feature = "boot-actuators")] - if data_len > 0 && cdb.first() == Some(&0x2A) { - mid_write::wedge_if_staged(Phase::StatusOwed); - } - let csw_phys = dma.device_addr() + (dev.block + MSC_CSW) as u64; - super::super::zero_dma(dma, dev.block + MSC_CSW, CSW_LEN as usize); - let mut got = self.framed_phase(dev, true, csw_phys, CSW_LEN, Phase::Status, &open); - if let Err(Broke::Code { code: CC_STALL, .. }) = got { - // The spec's one legal retry: the device may stall the status - // phase once. - if !self.restart_bulk(dev, true) { - return Err(Broke::Stall { phase: "status" }); + bot::Act::Restart { pipe, then } => { + let took = self.restart_bulk(dev, pipe == Pipe::In); + // The recovery rebuilt the ring, so the point the account + // reads is republished before anything else reaches the + // controller. + if took { + open.at(then, &dev.in_ring, &dev.out_ring); + } + bot::Answer::Restarted(took) + } + }; + match trip.answered(answer) { + bot::Next::Act(next, then) => (trip, act) = (next, then), + bot::Next::Csw(due) => { + #[cfg(feature = "stack-witness")] + block_witness_holds(dev, entered_with); + let mut csw = [0u8; CSW_LEN]; + dma.copy_to(dev.block + MSC_CSW, &mut csw); + return due.judge(&csw); + } + bot::Next::Broke(broke) => return Err(broke), } - open.at(Phase::StatusOwed, &dev.in_ring, &dev.out_ring); - super::super::zero_dma(dma, dev.block + MSC_CSW, CSW_LEN as usize); - got = self.framed_phase(dev, true, csw_phys, CSW_LEN, Phase::Status, &open); - } - got?; - - #[cfg(feature = "stack-witness")] - block_witness_holds(dev, entered_with); - // Unaligned again; bounded by the CSW_LEN subview, exclusive because - // `framed_phase` returned `Ok`. Every field is checked below, never - // believed. - let csw = dma.subview(dev.block + MSC_CSW, CSW_LEN as usize).unaligned(); - let (signature, csw_tag, residue, status) = ( - u32::from_le(csw.read::(0)), - u32::from_le(csw.read::(4)), - u32::from_le(csw.read::(8)), - csw.read::(12), - ); - if signature != CSW_SIGNATURE { - return Err(Broke::Csw { - what: "signature", - got: signature, - want: CSW_SIGNATURE, - status, - residue, - }); - } - // Accepting a mismatched tag would attribute one command's status - // to another — a write reporting the read before it as success. - if csw_tag != tag { - return Err(Broke::Csw { what: "tag", got: csw_tag, want: tag, status, residue }); - } - if residue > data_len { - return Err(Broke::Residue { unmoved: residue, of: data_len }); - } - match status { - // Neither account is trusted alone: a caller may read only what - // both the device and the controller say arrived. - 0 => Ok(Bot::Done { delivered: moved.min(data_len - residue) }), - 1 => Ok(Bot::Failed), - _ => Err(Broke::PhaseError), } } @@ -1826,7 +1595,7 @@ impl XhciController { if !recovered { return Climbed::Failed; } - match self.bot(dev, &TEST_UNIT_READY, 6, None, false, Asks::Verification(Rung::ClassReset)) { + match self.bot(dev, &Cdb::TEST_UNIT_READY, None, Asks::Verification(Rung::ClassReset)) { Ok(answer) => { log!("usb-storage: {slot} Reset Recovery took: the device answered TEST UNIT \ READY under its own tag {:#x}", dev.tag); @@ -2151,11 +1920,8 @@ pub(in crate::drivers::xhci) fn bind( in_ring, out_ring, tag: 0, - logical_block_bytes: 0, - sectors_per_block: 0, - blocks: 0, - breaks: 0, - climbed: None, + geometry: Geometry::NONE, + run: Run::NONE, failed: false, slot_goes: None, no_write_cache: false, @@ -2203,8 +1969,8 @@ pub(in crate::drivers::xhci) fn bind( log!("usb-storage: disk {index} came back on port {} slot {slot_id} as the same device \ (USB {:04x}:{:04x}, serial number {}, {} blocks of {} B), msc_block +{:#x}; its \ volume carries on{}", - u32::from(port_idx) + 1, usb.vendor, usb.product, dev.identity.serial, dev.blocks, - dev.logical_block_bytes, block, + u32::from(port_idx) + 1, usb.vendor, usb.product, dev.identity.serial, dev.geometry.blocks, + dev.geometry.sector_bytes, block, if owed { OWED_A_FLUSH } else { "" }); ctrl.msc[at].disk = Some(Disk { index, dev }); return Bind::Bound; @@ -2213,9 +1979,9 @@ pub(in crate::drivers::xhci) fn bind( log!( "usb-storage: disk {index} ready on slot {slot_id}, {} blocks of {} B \ ({} MiB), msc_block +{:#x}", - dev.blocks, - dev.logical_block_bytes, - dev.blocks * HOST_BLOCK as u64 / (1024 * 1024), + dev.geometry.blocks, + dev.geometry.sector_bytes, + dev.geometry.blocks * u64::from(HOST_BLOCK) / (1024 * 1024), block ); ctrl.msc[at].disk = Some(Disk { index, dev }); @@ -2271,148 +2037,114 @@ enum Up { Refused, } -/// TEST UNIT READY, INQUIRY and READ CAPACITY: everything between a configured -/// interface and a disk with a size. +/// Everything between a configured interface and a disk with a size, as +/// `toyos_xhci::scsi::BringUp` asks for it. fn bring_up(ctrl: &mut XhciController, dev: &mut MscDevice) -> Up { - // Drives the transport directly, not `scsi`: NOT READY is expected, not - // an error, so it must not log per attempt, and fetching sense also - // clears the condition on a device still spinning up. - let give_up = crate::clock::nanos_since_boot() + READY_BUDGET.nanos(); - let mut sense = (0u8, 0u8, 0u8); - let mut ready = false; + let slot = dev.slot_id; + let dma = ctrl.dma(); + let scratch = dma.subview(dev.block + MSC_SCRATCH, MSC_SCRATCH_LEN); + // No caller budget here: bring-up isn't an operation with a + // `BlockDevice` handle to answer — it answers only to `READY_BUDGET` and + // `USB_TIMEOUT_NS`. + let until = Deadline::never(); + let mut pending = None; + let mut read = [0u8; MSC_SCRATCH_LEN]; + let (mut up, mut ask) = BringUp::begin(crate::clock::nanos_since_boot() + READY_BUDGET.nanos()); loop { - match ctrl.bot(dev, &TEST_UNIT_READY, 6, None, false, Asks::Command) { - Ok(Bot::Done { .. }) => { - ready = true; - break; - } - Ok(Bot::Failed) => sense = ctrl.request_sense(dev), - Err(broke) => { - log!("usb-storage: slot {} broke on TEST UNIT READY: {broke}", dev.slot_id); + let heard = match ask { + scsi::Ask::TestUnitReady => match ctrl.bot(dev, &Cdb::TEST_UNIT_READY, None, Asks::Command) { + Ok(Bot::Done { .. }) => Heard::Good, + Ok(Bot::Failed) => Heard::CheckCondition, + Err(why) => { + let broke = Told(&why); + log!("usb-storage: slot {} broke on TEST UNIT READY: {broke}", dev.slot_id); + pending = Some(why); + Heard::Broke + } + }, + scsi::Ask::RequestSense => Heard::Sense(ctrl.request_sense(dev)), + scsi::Ask::Recover => { + let why = pending.take().expect("a recovery is asked for only after a break"); ctrl.after_break.open(ctrl.bulk_began, AFTER_BREAK); // A rung ends on this same command answered, so the run of // breaks it counted is over when it says the device is in step. - if ctrl.climb_until_in_step(dev, broke.event(), Left::Elsewhere) { - dev.breaks = 0; - dev.climbed = None; + if ctrl.climb_until_in_step(dev, why.event(), Left::Elsewhere) { + dev.run.over(); } ctrl.after_break = AfterBreak::CLOSED; + Heard::Recovered { offline: dev.failed } } - } - if dev.failed || crate::clock::nanos_since_boot() >= give_up { - break; - } - } - if !ready { - log!("usb-storage: slot {} never became ready, sense {:#04x}/{:#04x}/{:#04x}", - dev.slot_id, sense.0, sense.1, sense.2); - return if dev.failed { Up::Refused } else { Up::NotReady }; - } - - let dma = ctrl.dma(); - let scratch = dma.subview(dev.block + MSC_SCRATCH, MSC_SCRATCH_LEN); - // No caller budget here: bring-up isn't an operation with a - // `BlockDevice` handle to answer — it answers only to `READY_BUDGET` and - // `USB_TIMEOUT_NS`. - let until = Deadline::never(); - let read_scratch = |ctrl: &mut XhciController, - dev: &mut MscDevice, - cdb: &[u8], - cdb_len: u8, - want: u32, - out: &mut [u8]| { - scratch.zero(); - // `subview` refuses a command asking for more than the scratch - // buffer holds. Each command of a bind is a call of its own. - let answer = ctrl.scsi(dev, cdb, cdb_len, Some(scratch.subview(0, want as usize)), true, until); - ctrl.after_break = AfterBreak::CLOSED; - match answer { - Scsi::Ok { delivered } if delivered as usize >= out.len() => { - dma.copy_to(dev.block + MSC_SCRATCH, out); - true + scsi::Ask::Read(query) => { + let (cdb, len) = (query.cdb(), query.allocation()); + scratch.zero(); + // `subview` refuses a command asking for more than the scratch + // buffer holds. Each command of a bind is a call of its own. + let outcome = ctrl.scsi(dev, &cdb, Some(scratch.subview(0, len)), until); + ctrl.after_break = AfterBreak::CLOSED; + match outcome { + Outcome::Ok { delivered } => { + dma.copy_to(dev.block + MSC_SCRATCH, &mut read[..len]); + Heard::Data { bytes: &read[..len], delivered } + } + Outcome::Refused(sense) => { + log_refusal(&cdb, sense); + Heard::Unanswered + } + Outcome::Broken | Outcome::Budget => Heard::Unanswered, + } } - Scsi::Refused { key, asc, ascq } => { - log_refusal(cdb, key, asc, ascq); - false + }; + let end = match up.heard(heard, crate::clock::nanos_since_boot()) { + scsi::Next::Ask(next, then) => { + (up, ask) = (next, then); + continue; } - _ => false, - } - }; - - let mut inquiry = [0u8; 36]; - if !read_scratch(ctrl, dev, &[0x12u8, 0, 0, 0, 36, 0], 6, 36, &mut inquiry) { - log!("usb-storage: slot {} would not answer INQUIRY", dev.slot_id); - return Up::Refused; - } - let peripheral = inquiry[0] & 0x1F; - if peripheral != 0 { - log!("usb-storage: slot {} is SCSI peripheral type {peripheral:#04x}, not a disk", - dev.slot_id); - return Up::Refused; - } - log!("usb-storage: slot {} vendor {} product {}", dev.slot_id, - Printable(&inquiry[8..16]), Printable(&inquiry[16..32])); - dev.identity.inquiry.copy_from_slice(&inquiry[8..36]); - - // READ CAPACITY(10) reports an all-ones last LBA when the disk needs the - // 16-byte form to describe its size. - let mut cap10 = [0u8; 8]; - if !read_scratch(ctrl, dev, &[0x25u8, 0, 0, 0, 0, 0, 0, 0, 0, 0], 10, 8, &mut cap10) { - log!("usb-storage: slot {} would not answer READ CAPACITY(10)", dev.slot_id); - return Up::Refused; - } - let (last_lba, block_bytes) = if u32::from_be_bytes([cap10[0], cap10[1], cap10[2], cap10[3]]) - == u32::MAX - { - let mut cap16 = [0u8; 12]; - let cdb = [0x9Eu8, 0x10, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 32, 0, 0]; - if !read_scratch(ctrl, dev, &cdb, 16, 32, &mut cap16) { - log!("usb-storage: slot {} would not answer READ CAPACITY(16)", dev.slot_id); - return Up::Refused; - } - ( - u64::from_be_bytes([ - cap16[0], cap16[1], cap16[2], cap16[3], cap16[4], cap16[5], cap16[6], cap16[7], - ]), - u32::from_be_bytes([cap16[8], cap16[9], cap16[10], cap16[11]]), - ) - } else { - ( - u32::from_be_bytes([cap10[0], cap10[1], cap10[2], cap10[3]]) as u64, - u32::from_be_bytes([cap10[4], cap10[5], cap10[6], cap10[7]]), - ) - }; - - // A zero or >4096 block size divides by zero below (`4096 / - // block_bytes`); the allowed set is which sizes divide the 4 KiB host - // block. - if !matches!(block_bytes, 512 | 1024 | 2048 | 4096) { - log!("usb-storage: slot {} reports {block_bytes}-byte blocks; this driver \ - serves 4096-byte blocks and needs 512..=4096", dev.slot_id); - return Up::Refused; - } - // READ/WRITE(10) carry a 32-bit LBA; serving the first 2 TiB of a - // bigger disk would silently truncate it. - if last_lba > u32::MAX as u64 { - log!("usb-storage: slot {} has {} sectors; this driver issues READ(10) and \ - addresses 2^32", dev.slot_id, last_lba as u128 + 1); - return Up::Refused; - } - let sectors = last_lba + 1; - let sectors_per_block = HOST_BLOCK / block_bytes; - let blocks = sectors / sectors_per_block as u64; - if blocks == 0 { - log!("usb-storage: slot {} holds {sectors} sectors of {block_bytes} B, less \ - than one 4096-byte block", dev.slot_id); - return Up::Refused; - } - - dev.logical_block_bytes = block_bytes; - dev.sectors_per_block = sectors_per_block; - dev.blocks = blocks; - dev.identity.sectors = sectors; - dev.identity.sector_bytes = block_bytes; - Up::Ready + scsi::Next::Disk(next, inquiry, then) => { + log!("usb-storage: slot {slot} vendor {} product {}", + Printable(inquiry.vendor()), Printable(inquiry.product())); + dev.identity.inquiry = inquiry.0; + (up, ask) = (next, then); + continue; + } + scsi::Next::Up(end) => end, + }; + return match end { + scsi::Up::Ready(geometry) => { + dev.geometry = geometry; + dev.identity.sectors = geometry.sectors; + dev.identity.sector_bytes = geometry.sector_bytes; + Up::Ready + } + scsi::Up::Unready { sense, offline } => { + log!("usb-storage: slot {slot} never became ready, sense {sense}"); + if offline { Up::Refused } else { Up::NotReady } + } + scsi::Up::Refused(why) => { + match why { + Refusal::Unanswered(query) => { + log!("usb-storage: slot {slot} would not answer {}", query.named()); + } + Refusal::NotADisk(peripheral) => { + log!("usb-storage: slot {slot} is SCSI peripheral type {peripheral:#04x}, \ + not a disk"); + } + Refusal::SectorSize(bytes) => { + log!("usb-storage: slot {slot} reports {bytes}-byte blocks; this driver \ + serves 4096-byte blocks and needs 512..=4096"); + } + Refusal::PastRead10 { last_lba } => { + log!("usb-storage: slot {slot} has {} sectors; this driver issues READ(10) \ + and addresses 2^32", u128::from(last_lba) + 1); + } + Refusal::LessThanABlock { sectors, sector_bytes } => { + log!("usb-storage: slot {slot} holds {sectors} sectors of {sector_bytes} B, \ + less than one 4096-byte block"); + } + } + Up::Refused + } + }; + } } /// The serial number string a device's iSerialNumber names (USB 2.0 §9.6.1), @@ -2448,32 +2180,13 @@ fn read_serial(ctrl: &mut XhciController, dev: &mut MscDevice, index: u8) -> Ser Some((arrived, delivered)) }; let Some((languages, delivered)) = get(ctrl, 0, 0) else { return Serial::Unread }; - // Descriptor zero's first LANGID; a device offering none names no string. - if delivered < 4 || languages[0] < 4 || languages[1] != 3 { - return Serial::Unread; - } - let language = u16::from_le_bytes([languages[2], languages[3]]); + let Some(language) = identity::first_language(&languages[..delivered]) else { return Serial::Unread }; match get(ctrl, index, language) { Some((arrived, delivered)) => Serial::from_descriptor(&arrived[..delivered]), None => Serial::Unread, } } -/// A device-supplied ASCII field, rendered without letting it choose what the -/// log looks like. -struct Printable<'a>(&'a [u8]); - -impl core::fmt::Display for Printable<'_> { - fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - f.write_str("\"")?; - let mut utf8 = [0u8; 4]; - for &b in self.0 { - let c = if (0x20..0x7F).contains(&b) && b != b'"' { b as char } else { '.' }; - f.write_str(c.encode_utf8(&mut utf8))?; - } - f.write_str("\"") - } -} /// Read `count` 4 KiB blocks at `lba`. On `Err` the transfer did not happen /// and `buf` holds nothing the caller may believe. /// The caller must be inside a block-device operation diff --git a/src/sourcegate.rs b/src/sourcegate.rs index 21e9f5e583..17c5833847 100644 --- a/src/sourcegate.rs +++ b/src/sourcegate.rs @@ -238,10 +238,7 @@ const BANS: &[Ban] = &[ Ban { needle: ": u32 = 4096", why: "as above, in the other width", - allowed: &[ - // The block size this driver reads a disk in. - ("kernel/src/drivers/xhci/wait/msc.rs", 1), - ], + allowed: &[], }, ]; diff --git a/toyos-xhci/src/bot.rs b/toyos-xhci/src/bot.rs index f10a4af72d..7a292a8a97 100644 --- a/toyos-xhci/src/bot.rs +++ b/toyos-xhci/src/bot.rs @@ -10,6 +10,17 @@ //! The phases are here and their effects are the driver's, because the reader //! is the reset path: it runs where no lock may be taken, so it reads the phase //! out of an atomic rather than out of the driver. +//! +//! [`RoundTrip`] is the round trip as a machine the driver answers: which +//! transfer is owed, what each completion means, the one legal stall retry, and +//! whether the CSW is believed. The driver queues and waits; nothing here +//! touches a ring, so a driver that waits in place and one that gives the CPU +//! back between transfers drive the same order. + +use crate::job::{CC_SHORT_PACKET, CC_SUCCESS}; +use crate::ladder::{self, Left}; +use crate::reset_recovery::Pipe; +use crate::scsi::Cdb; /// Where one Bulk-Only round trip stands, published by the driver as it walks /// the three phases. @@ -93,6 +104,267 @@ impl core::fmt::Display for Phase { } } +/// Completion code 6 (xHCI 1.2 Table 6-90): the device STALLed the transfer. +pub const CC_STALL: u32 = 6; + +pub const CBW_LEN: usize = 31; +pub const CSW_LEN: usize = 13; +const CBW_SIGNATURE: u32 = 0x4342_5355; +const CSW_SIGNATURE: u32 = 0x5342_5355; + +/// The Command Block Wrapper (§5.1) carrying `cdb` under `tag`, for a data +/// phase of `data_len` bytes, to LUN 0: this driver binds one logical unit. +pub fn cbw(tag: u32, data_len: u32, cdb: &Cdb) -> [u8; CBW_LEN] { + let mut out = [0u8; CBW_LEN]; + out[0..4].copy_from_slice(&CBW_SIGNATURE.to_le_bytes()); + out[4..8].copy_from_slice(&tag.to_le_bytes()); + out[8..12].copy_from_slice(&data_len.to_le_bytes()); + out[12] = if cdb.data_in() { 0x80 } else { 0x00 }; + let bytes = cdb.bytes(); + out[14] = bytes.len() as u8; + out[15..15 + bytes.len()].copy_from_slice(bytes); + out +} + +/// How one round trip ended, where the device answered in step. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum Bot { + /// CSW status 0; `delivered` is the smaller of what the controller moved + /// and what the device says it did not leave unmoved — else stale data + /// from an earlier LBA leaks. Never more than the transfer. + Done { delivered: u32 }, + /// CSW status 1: the device understood and refused. Sense data says why. + Failed, +} + +/// Why a round trip could not be completed; what happened decides which +/// recovery is legal. `W` is the driver's reason for a silence. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum Broke { + /// The controller reported this completion code for the phase, on this + /// pipe. + Code { phase: Phase, code: u32, pipe: Pipe }, + /// Nothing came back for the phase. + Silence { phase: Phase, why: W }, + /// The port reads empty: the device is no longer on the bus. + Gone { phase: Phase }, + /// A CBW or CSW moved the wrong byte count; both are fixed length, so + /// short is not a short transfer. + Short { phase: Phase, moved: u32, wanted: u32 }, + /// The endpoint stalled and its restart did not take. + Stall { phase: Phase }, + /// CSW status 2, which leaves both endpoints Running. + PhaseError, + /// The CSW named somebody else's transfer; the status and residue are the + /// rest of what the device said, and tell a status made for an abandoned + /// command from one made for nothing. + Csw { what: &'static str, got: u32, want: u32, status: u8, residue: u32 }, + /// More bytes claimed unmoved than the transfer had; believing it would + /// underflow the byte count every caller uses. + Residue { unmoved: u32, of: u32 }, +} + +impl Broke { + /// Where the break left the device ([`ladder::left`]); `data_out` is + /// whether the command that broke sends data. + pub fn left(&self, data_out: bool) -> Left { + let phase = match self { + Self::Code { phase, .. } + | Self::Silence { phase, .. } + | Self::Gone { phase } + | Self::Stall { phase } + | Self::Short { phase, .. } => *phase, + Self::PhaseError | Self::Csw { .. } | Self::Residue { .. } => Phase::Status, + }; + ladder::left(phase, data_out) + } + + /// The transfer event that ended the round trip, where one did: what a + /// quiesce takes ahead of the pipe's Endpoint State field. + pub fn event(&self) -> Option<(Pipe, u32)> { + match self { + Self::Code { code, pipe, .. } => Some((*pipe, *code)), + _ => None, + } + } +} + +/// What the round trip asks of the driver next. +/// +/// Before [`Act::Data`] the device holds a CBW and nothing is queued for it +/// ([`Phase::DataOwed`]), and before [`Act::Status`] it holds a CSW nothing has +/// asked for ([`Phase::StatusOwed`]); the driver publishes both. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum Act { + /// The CBW, [`CBW_LEN`] bytes on the Bulk-Out. + Command, + /// The data phase, the whole of its region, on this pipe. + Data(Pipe), + /// Into a zeroed buffer: the CSW, [`CSW_LEN`] bytes on the Bulk-In. + Status, + /// The pipe stalled: back to running, and once it is, `then` is the phase + /// published over its rebuilt ring. + Restart { pipe: Pipe, then: Phase }, +} + +/// How the driver's last [`Act`] ended. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum Answer { + /// The transfer's completion code, and the bytes it left unmoved. + Moved { code: u32, residue: u32 }, + Silent(W), + Gone, + /// Whether the restart took. + Restarted(bool), +} + +/// Where a round trip stands. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +enum At { + Command, + Data, + /// The data phase stalled with this many bytes unmoved. + DataRestart { unmoved: u32 }, + Status { retried: bool }, + StatusRestart, +} + +/// One Bulk-Only round trip: command block out, data, status in. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub struct RoundTrip { + at: At, + tag: u32, + data_len: u32, + data_in: bool, + /// What the controller says reached the buffer, checked against the CSW's + /// residue. + moved: u32, +} + +/// Where a round trip goes after an answer. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum Next { + Act(RoundTrip, Act), + /// All thirteen bytes of a CSW arrived: [`CswDue::judge`] them. + Csw(CswDue), + Broke(Broke), +} + +impl RoundTrip { + /// The round trip of the CBW sent under `tag` for `cdb`, with a data + /// phase of `data_len` bytes. + pub fn begin(tag: u32, data_len: u32, cdb: &Cdb) -> (Self, Act) { + let trip = Self { at: At::Command, tag, data_len, data_in: cdb.data_in(), moved: 0 }; + (trip, Act::Command) + } + + fn status(self, retried: bool) -> (Self, Act) { + (Self { at: At::Status { retried }, ..self }, Act::Status) + } + + /// The last act ended with `answer`. + pub fn answered(self, answer: Answer) -> Next { + let data_pipe = Pipe::of(self.data_in); + match (self.at, answer) { + (At::Command, answer) => match framed(Phase::Command, Pipe::Out, CBW_LEN as u32, answer) { + Err(broke) => Next::Broke(broke), + Ok(()) if self.data_len > 0 => Next::Act(Self { at: At::Data, ..self }, Act::Data(data_pipe)), + Ok(()) => { + let (trip, act) = self.status(false); + Next::Act(trip, act) + } + }, + (At::Data, Answer::Moved { code: CC_SUCCESS | CC_SHORT_PACKET, residue }) => { + let (trip, act) = Self { moved: self.data_len.saturating_sub(residue), ..self }.status(false); + Next::Act(trip, act) + } + // A stalled data phase is ordinary — an unsupported command, a read + // past the end — and the status that follows its recovery turns it + // into a clean refusal (§6.7.2). + (At::Data, Answer::Moved { code: CC_STALL, residue }) => Next::Act( + Self { at: At::DataRestart { unmoved: residue }, ..self }, + Act::Restart { pipe: data_pipe, then: Phase::Data }, + ), + (At::Data, Answer::Moved { code, .. }) => { + Next::Broke(Broke::Code { phase: Phase::Data, code, pipe: data_pipe }) + } + (At::Data, Answer::Silent(why)) => Next::Broke(Broke::Silence { phase: Phase::Data, why }), + (At::Data, Answer::Gone) => Next::Broke(Broke::Gone { phase: Phase::Data }), + (At::DataRestart { unmoved }, Answer::Restarted(true)) => { + let (trip, act) = Self { moved: self.data_len.saturating_sub(unmoved), ..self }.status(false); + Next::Act(trip, act) + } + (At::DataRestart { .. }, Answer::Restarted(false)) => Next::Broke(Broke::Stall { phase: Phase::Data }), + // The class's one legal retry: a device may STALL the status + // phase once (§5.3.3, Figure 2). + (At::Status { retried: false }, Answer::Moved { code: CC_STALL, .. }) => Next::Act( + Self { at: At::StatusRestart, ..self }, + Act::Restart { pipe: Pipe::In, then: Phase::StatusOwed }, + ), + (At::Status { .. }, answer) => match framed(Phase::Status, Pipe::In, CSW_LEN as u32, answer) { + Ok(()) => Next::Csw(CswDue { tag: self.tag, data_len: self.data_len, moved: self.moved }), + Err(broke) => Next::Broke(broke), + }, + (At::StatusRestart, Answer::Restarted(true)) => { + let (trip, act) = self.status(true); + Next::Act(trip, act) + } + (At::StatusRestart, Answer::Restarted(false)) => Next::Broke(Broke::Stall { phase: Phase::Status }), + (at, _) => panic!("a round trip at {at:?} was answered for an act it did not ask for"), + } + } +} + +/// A CBW or CSW leg, which the device must take or give in full. Short Packet +/// is how the controller reports a sub-maximum-packet transfer — a 13-byte +/// CSW on a 512-byte endpoint — so only the residue says it all moved. +fn framed(phase: Phase, pipe: Pipe, len: u32, answer: Answer) -> Result<(), Broke> { + match answer { + Answer::Moved { code: CC_SUCCESS | CC_SHORT_PACKET, residue: 0 } => Ok(()), + Answer::Moved { code: CC_SUCCESS | CC_SHORT_PACKET, residue } => { + Err(Broke::Short { phase, moved: len.saturating_sub(residue), wanted: len }) + } + Answer::Moved { code, .. } => Err(Broke::Code { phase, code, pipe }), + Answer::Silent(why) => Err(Broke::Silence { phase, why }), + Answer::Gone => Err(Broke::Gone { phase }), + Answer::Restarted(_) => panic!("a {phase} transfer was answered as a restart"), + } +} + +/// A CSW that arrived whole, owed its judgement. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub struct CswDue { + tag: u32, + data_len: u32, + moved: u32, +} + +impl CswDue { + /// Every field checked (§6.3), never believed. + pub fn judge(self, csw: &[u8; CSW_LEN]) -> Result> { + let word = |at: usize| u32::from_le_bytes([csw[at], csw[at + 1], csw[at + 2], csw[at + 3]]); + let (signature, tag, residue, status) = (word(0), word(4), word(8), csw[12]); + if signature != CSW_SIGNATURE { + return Err(Broke::Csw { what: "signature", got: signature, want: CSW_SIGNATURE, status, residue }); + } + // A mismatched tag would attribute one command's status to another — a + // write reporting the read before it as success. + if tag != self.tag { + return Err(Broke::Csw { what: "tag", got: tag, want: self.tag, status, residue }); + } + if residue > self.data_len { + return Err(Broke::Residue { unmoved: residue, of: self.data_len }); + } + match status { + // Neither account is trusted alone: a caller may read only what + // both the device and the controller say arrived. + 0 => Ok(Bot::Done { delivered: self.moved.min(self.data_len - residue) }), + 1 => Ok(Bot::Failed), + _ => Err(Broke::PhaseError), + } + } +} + #[cfg(test)] mod tests { use super::*; @@ -149,4 +421,273 @@ mod tests { } } } + + /// The driver's reason for a silence, which the machine only carries. + #[derive(Clone, Copy, PartialEq, Eq, Debug)] + struct Why; + + const TAG: u32 = 0x0102_0304; + const WHOLE: Answer = Answer::Moved { code: CC_SUCCESS, residue: 0 }; + + fn read(sectors: u16) -> Cdb { + Cdb::transfer(false, 0, sectors) + } + + fn write(sectors: u16) -> Cdb { + Cdb::transfer(true, 0, sectors) + } + + /// A CSW as §5.2 lays it out. + fn csw(tag: u32, residue: u32, status: u8) -> [u8; CSW_LEN] { + let mut out = [0u8; CSW_LEN]; + out[0..4].copy_from_slice(b"USBS"); + out[4..8].copy_from_slice(&tag.to_le_bytes()); + out[8..12].copy_from_slice(&residue.to_le_bytes()); + out[12] = status; + out + } + + /// Sized past the longest route, so one that grew runs off it. + const LONGEST: usize = 8; + + struct Walk { + acts: [Option; LONGEST], + end: Result>, + } + + impl Walk { + fn acts(&self) -> impl Iterator + '_ { + self.acts.iter().map_while(|a| *a) + } + + fn count(&self, act: Act) -> usize { + self.acts().filter(|a| *a == act).count() + } + } + + /// A round trip driven to its end: each act answered by `device`, and the + /// CSW, once one arrives whole, is `status`. + fn walk(cdb: Cdb, data_len: u32, mut device: impl FnMut(Act) -> Answer, status: [u8; CSW_LEN]) -> Walk { + let (mut trip, first) = RoundTrip::begin(TAG, data_len, &cdb); + let mut acts = [None; LONGEST]; + let mut act = first; + for slot in &mut acts { + *slot = Some(act); + match trip.answered(device(act)) { + Next::Act(next, then) => (trip, act) = (next, then), + Next::Csw(due) => return Walk { acts, end: due.judge(&status) }, + Next::Broke(broke) => return Walk { acts, end: Err(broke) }, + } + } + panic!("a round trip that does not end: {acts:?}"); + } + + /// §5.1's layout, byte for byte: "USBC", the tag and the length little + /// endian, the direction in bit 7 of the flags, LUN 0, and the CDB's own + /// length before it. + #[test] + fn a_cbw_is_laid_out_as_the_class_defines_it() { + let got = cbw(TAG, 0x1000, &read(8)); + let want: [u8; CBW_LEN] = [ + b'U', b'S', b'B', b'C', 0x04, 0x03, 0x02, 0x01, 0x00, 0x10, 0x00, 0x00, 0x80, 0x00, 10, + 0x28, 0, 0, 0, 0, 0, 0, 0, 8, 0, 0, 0, 0, 0, 0, 0, + ]; + assert_eq!(got, want); + assert_eq!(cbw(TAG, 0x1000, &write(8))[12], 0x00, "Data-Out"); + let tur = cbw(TAG, 0, &Cdb::TEST_UNIT_READY); + assert_eq!((tur[12], tur[14]), (0x00, 6)); + assert!(tur[15..].iter().all(|b| *b == 0)); + } + + #[test] + fn a_command_with_no_data_is_its_command_block_and_its_status() { + let walk = walk(Cdb::TEST_UNIT_READY, 0, |_| WHOLE, csw(TAG, 0, 0)); + assert!(walk.acts().eq([Act::Command, Act::Status])); + assert_eq!(walk.end, Ok(Bot::Done { delivered: 0 })); + } + + #[test] + fn a_data_phase_runs_on_the_pipe_its_command_names() { + let walk_in = walk(read(1), 512, |_| WHOLE, csw(TAG, 0, 0)); + assert!(walk_in.acts().eq([Act::Command, Act::Data(Pipe::In), Act::Status])); + assert_eq!(walk_in.end, Ok(Bot::Done { delivered: 512 })); + let walk_out = walk(write(1), 512, |_| WHOLE, csw(TAG, 0, 0)); + assert!(walk_out.acts().eq([Act::Command, Act::Data(Pipe::Out), Act::Status])); + } + + /// Neither account alone: what the controller moved, and what the device + /// says it did not. + #[test] + fn what_is_delivered_is_what_both_the_controller_and_the_device_say_arrived() { + let short = |act| match act { + Act::Data(_) => Answer::Moved { code: CC_SHORT_PACKET, residue: 100 }, + _ => WHOLE, + }; + assert_eq!(walk(read(1), 512, short, csw(TAG, 0, 0)).end, Ok(Bot::Done { delivered: 412 })); + assert_eq!(walk(read(1), 512, |_| WHOLE, csw(TAG, 200, 0)).end, Ok(Bot::Done { delivered: 312 })); + assert_eq!(walk(read(1), 512, short, csw(TAG, 50, 0)).end, Ok(Bot::Done { delivered: 412 })); + } + + /// §6.7.2: the device STALLs a data phase it will not finish, and the + /// status after the pipe's recovery says why. + #[test] + fn a_stalled_data_phase_is_restarted_and_its_status_read() { + let stalls = |act| match act { + Act::Data(_) => Answer::Moved { code: CC_STALL, residue: 512 }, + Act::Restart { .. } => Answer::Restarted(true), + _ => WHOLE, + }; + let walk = walk(read(1), 512, stalls, csw(TAG, 512, 1)); + assert!(walk.acts().eq([ + Act::Command, + Act::Data(Pipe::In), + Act::Restart { pipe: Pipe::In, then: Phase::Data }, + Act::Status, + ])); + assert_eq!(walk.end, Ok(Bot::Failed)); + } + + #[test] + fn a_data_phase_whose_pipe_will_not_restart_breaks_as_a_stall() { + let stuck = |act| match act { + Act::Data(_) => Answer::Moved { code: CC_STALL, residue: 0 }, + Act::Restart { .. } => Answer::Restarted(false), + _ => WHOLE, + }; + let walk = walk(write(1), 512, stuck, csw(TAG, 0, 0)); + assert_eq!(walk.end, Err(Broke::Stall { phase: Phase::Data })); + assert_eq!(walk.count(Act::Status), 0, "no status is asked of a pipe that did not restart"); + } + + /// §5.3.3: a STALLed CSW is asked for once more, and only once. + #[test] + fn a_stalled_status_is_asked_for_again_once() { + let mut stalls = 1; + let once = walk(Cdb::TEST_UNIT_READY, 0, |act| match act { + Act::Status if stalls > 0 => { + stalls -= 1; + Answer::Moved { code: CC_STALL, residue: CSW_LEN as u32 } + } + Act::Restart { .. } => Answer::Restarted(true), + _ => WHOLE, + }, csw(TAG, 0, 0)); + assert!(once.acts().eq([ + Act::Command, + Act::Status, + Act::Restart { pipe: Pipe::In, then: Phase::StatusOwed }, + Act::Status, + ])); + assert_eq!(once.end, Ok(Bot::Done { delivered: 0 })); + + let always = walk(Cdb::TEST_UNIT_READY, 0, |act| match act { + Act::Status => Answer::Moved { code: CC_STALL, residue: CSW_LEN as u32 }, + Act::Restart { .. } => Answer::Restarted(true), + _ => WHOLE, + }, csw(TAG, 0, 0)); + assert_eq!(always.count(Act::Status), 2); + assert_eq!(always.end, Err(Broke::Code { phase: Phase::Status, code: CC_STALL, pipe: Pipe::In })); + + let stuck = walk(Cdb::TEST_UNIT_READY, 0, |act| match act { + Act::Status => Answer::Moved { code: CC_STALL, residue: CSW_LEN as u32 }, + Act::Restart { .. } => Answer::Restarted(false), + _ => WHOLE, + }, csw(TAG, 0, 0)); + assert_eq!(stuck.end, Err(Broke::Stall { phase: Phase::Status })); + } + + /// A CBW or CSW is fixed length, so a short one is a break and not a + /// short transfer — and a Short Packet completion that moved it all is not. + #[test] + fn a_command_or_status_block_that_moved_short_breaks() { + let short_cbw = walk(read(1), 512, |act| match act { + Act::Command => Answer::Moved { code: CC_SUCCESS, residue: 1 }, + _ => WHOLE, + }, csw(TAG, 0, 0)); + assert_eq!(short_cbw.end, Err(Broke::Short { phase: Phase::Command, moved: 30, wanted: 31 })); + let short_csw = walk(read(1), 512, |act| match act { + Act::Status => Answer::Moved { code: CC_SHORT_PACKET, residue: 4 }, + _ => WHOLE, + }, csw(TAG, 0, 0)); + assert_eq!(short_csw.end, Err(Broke::Short { phase: Phase::Status, moved: 9, wanted: 13 })); + let whole_csw = walk(read(1), 512, |act| match act { + Act::Status => Answer::Moved { code: CC_SHORT_PACKET, residue: 0 }, + _ => WHOLE, + }, csw(TAG, 0, 0)); + assert_eq!(whole_csw.end, Ok(Bot::Done { delivered: 512 })); + } + + /// Every leg's silence, disconnect and error is a break in that leg, on + /// the pipe it ran on, and nothing after it is asked. + #[test] + fn a_leg_that_fails_breaks_the_round_trip_where_it_failed() { + for (leg, phase, pipe) in [ + (Act::Command, Phase::Command, Pipe::Out), + (Act::Data(Pipe::Out), Phase::Data, Pipe::Out), + (Act::Status, Phase::Status, Pipe::In), + ] { + let failing = |answer: Answer| move |act| if act == leg { answer } else { WHOLE }; + let end = |answer| walk(write(1), 512, failing(answer), csw(TAG, 0, 0)); + assert_eq!(end(Answer::Silent(Why)).end, Err(Broke::Silence { phase, why: Why })); + assert_eq!(end(Answer::Gone).end, Err(Broke::Gone { phase })); + let babble = end(Answer::Moved { code: 3, residue: 0 }); + assert_eq!(babble.end, Err(Broke::Code { phase, code: 3, pipe })); + assert_eq!(babble.acts().last(), Some(leg)); + } + } + + /// §6.3: valid means the signature and the tag, meaningful means a status + /// the class defines and a residue no larger than the transfer. + #[test] + fn a_csw_is_believed_only_when_it_is_valid_and_meaningful() { + let judged = |csw: [u8; CSW_LEN]| walk(read(1), 512, |_| WHOLE, csw).end; + let mut unsigned = csw(TAG, 0, 0); + unsigned[0] = b'X'; + assert!(matches!(judged(unsigned), Err(Broke::Csw { what: "signature", .. }))); + assert_eq!( + judged(csw(TAG + 1, 7, 1)), + Err(Broke::Csw { what: "tag", got: TAG + 1, want: TAG, status: 1, residue: 7 }) + ); + assert_eq!(judged(csw(TAG, 513, 0)), Err(Broke::Residue { unmoved: 513, of: 512 })); + assert_eq!(judged(csw(TAG, 512, 0)), Ok(Bot::Done { delivered: 0 })); + assert_eq!(judged(csw(TAG, 0, 1)), Ok(Bot::Failed)); + for status in 2..=u8::MAX { + assert_eq!(judged(csw(TAG, 0, status)), Err(Broke::PhaseError), "status {status}"); + } + } + + /// A break before a Data-Out phase is done leaves its device owed the + /// data; a break in the status, or of a command that sends nothing, does + /// not. Only a transfer event says where a pipe stands. + #[test] + fn a_break_says_where_it_left_the_device_and_which_event_ended_it() { + let code = Broke::::Code { phase: Phase::Data, code: 4, pipe: Pipe::Out }; + assert_eq!(code.left(true), Left::OwedDataOut); + assert_eq!(code.left(false), Left::Elsewhere); + assert_eq!(code.event(), Some((Pipe::Out, 4))); + let gone = Broke::::Gone { phase: Phase::Command }; + assert_eq!(gone.left(true), Left::OwedDataOut); + assert_eq!(gone.event(), None); + for status in [ + Broke::::PhaseError, + Broke::Residue { unmoved: 2, of: 1 }, + Broke::Csw { what: "tag", got: 0, want: 1, status: 0, residue: 0 }, + Broke::Silence { phase: Phase::Status, why: Why }, + Broke::Stall { phase: Phase::Status }, + ] { + assert_eq!(status.left(true), Left::Elsewhere, "{status:?}"); + assert_eq!(status.event(), None, "{status:?}"); + } + } + + #[test] + #[should_panic(expected = "did not ask for")] + fn a_restart_answered_with_a_transfer_is_a_driver_bug() { + let (trip, _) = RoundTrip::begin(TAG, 512, &read(1)); + let Next::Act(trip, Act::Data(_)) = trip.answered::(WHOLE) else { panic!("no data act") }; + let Next::Act(trip, Act::Restart { .. }) = trip.answered::(Answer::Moved { code: CC_STALL, residue: 0 }) + else { + panic!("no restart") + }; + let _ = trip.answered::(WHOLE); + } } diff --git a/toyos-xhci/src/identity.rs b/toyos-xhci/src/identity.rs index a4ed176881..dba84e85f8 100644 --- a/toyos-xhci/src/identity.rs +++ b/toyos-xhci/src/identity.rs @@ -98,6 +98,15 @@ impl Serial { } } +/// The first LANGID string descriptor zero offers (USB 2.0 §9.6.7), as it +/// arrived; a device offering none names no string. +pub fn first_language(arrived: &[u8]) -> Option { + match *arrived { + [length, 3, lo, hi, ..] if length >= 4 => Some(u16::from_le_bytes([lo, hi])), + _ => None, + } +} + impl core::fmt::Display for Serial { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { match self { @@ -193,6 +202,16 @@ mod tests { const MS: Nanos = 1_000_000; + #[test] + fn a_language_is_read_only_from_a_string_descriptor_that_carries_one() { + assert_eq!(first_language(&[4, 3, 0x09, 0x04]), Some(0x0409)); + assert_eq!(first_language(&[6, 3, 0x07, 0x04, 0x09, 0x04]), Some(0x0407), "the first"); + assert_eq!(first_language(&[4, 3, 0x09]), None, "three bytes arrived"); + assert_eq!(first_language(&[2, 3, 0x09, 0x04]), None, "bLength says it carries none"); + assert_eq!(first_language(&[4, 2, 0x09, 0x04]), None, "not a string descriptor"); + assert_eq!(first_language(&[]), None); + } + fn serial(text: &str) -> Serial { let mut descriptor = [0u8; 256]; let mut at = 2; diff --git a/toyos-xhci/src/ladder.rs b/toyos-xhci/src/ladder.rs index 430b55e202..380d59896a 100644 --- a/toyos-xhci/src/ladder.rs +++ b/toyos-xhci/src/ladder.rs @@ -99,6 +99,49 @@ pub fn next(climbed: Option, left: Left) -> Rung { above.max(enters_at(left)) } +/// A device's run of breaks: how many, and the highest rung among them. +/// +/// **Per device, not per command**: the run it bounds is the device's, however +/// many callers and operations it is spread over, and only a completed round +/// trip ends it ([`Self::over`]). +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub struct Run { + breaks: u8, + climbed: Option, +} + +/// What one break costs the run. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub struct Climb { + pub rung: Rung, + /// The run's first rung is above the class reset: the device is owed the + /// Data-Out of the command that broke ([`enters_at`]). + pub skips_class_reset: bool, +} + +impl Run { + pub const NONE: Self = Self { breaks: 0, climbed: None }; + + /// Breaks in the run so far. + pub fn breaks(self) -> u8 { + self.breaks + } + + /// A break that left the device `left`: counted, and the rung it climbs. + pub fn broke(&mut self, left: Left) -> Climb { + self.breaks = self.breaks.saturating_add(1); + let rung = next(self.climbed, left); + let skips_class_reset = self.climbed.is_none() && rung != Rung::ClassReset; + self.climbed = Some(rung); + Climb { rung, skips_class_reset } + } + + /// A round trip completed: the run is over. How many breaks it held. + pub fn over(&mut self) -> u8 { + core::mem::replace(self, Self::NONE).breaks + } +} + /// One step of [`Rung::PortReset`], in the order taken. #[derive(Clone, Copy, PartialEq, Eq, Debug)] pub enum PortStep { @@ -237,6 +280,33 @@ mod tests { } } + /// The run is the device's: every break climbs above the last, the count + /// reaches [`MOST_BREAKS`] exactly where the run is offline, and a completed + /// round trip starts the next run at the bottom. + #[test] + fn a_run_climbs_one_rung_per_break_until_a_round_trip_ends_it() { + let mut run = Run::NONE; + assert_eq!(run.broke(Left::Elsewhere), Climb { rung: Rung::ClassReset, skips_class_reset: false }); + assert_eq!(run.broke(Left::Elsewhere), Climb { rung: Rung::PortReset, skips_class_reset: false }); + assert_eq!(run.broke(Left::Elsewhere).rung, Rung::Offline); + assert_eq!(run.breaks(), MOST_BREAKS); + assert_eq!(run.over(), MOST_BREAKS); + assert_eq!(run, Run::NONE); + assert_eq!(run.broke(Left::Elsewhere).rung, Rung::ClassReset, "a run begun again climbs from the bottom"); + } + + /// Said once, at the first rung of a run, and only when that rung is the + /// port reset. + #[test] + fn only_a_run_that_begins_owed_data_out_skips_the_class_reset() { + let mut run = Run::NONE; + assert_eq!(run.broke(Left::OwedDataOut), Climb { rung: Rung::PortReset, skips_class_reset: true }); + assert_eq!(run.broke(Left::OwedDataOut), Climb { rung: Rung::Offline, skips_class_reset: false }); + let mut run = Run::NONE; + run.broke(Left::Elsewhere); + assert!(!run.broke(Left::OwedDataOut).skips_class_reset, "the class reset was climbed"); + } + #[test] fn a_break_elsewhere_climbs_every_rung() { assert_eq!(next(None, Left::Elsewhere), Rung::ClassReset); diff --git a/toyos-xhci/src/lib.rs b/toyos-xhci/src/lib.rs index ffcfc24b0f..124d3e6e6a 100644 --- a/toyos-xhci/src/lib.rs +++ b/toyos-xhci/src/lib.rs @@ -25,6 +25,7 @@ pub mod recovery; pub mod reset_recovery; pub mod ring; pub mod scan; +pub mod scsi; pub use job::{Await, Outcome, Outstanding}; pub use port::{Effect, Gone, Nanos, PortState, Step}; diff --git a/toyos-xhci/src/scsi.rs b/toyos-xhci/src/scsi.rs new file mode 100644 index 0000000000..a7cd87c213 --- /dev/null +++ b/toyos-xhci/src/scsi.rs @@ -0,0 +1,859 @@ +//! The SCSI half of a Bulk-Only disk: the commands sent (SPC-4, SBC-3), what +//! each answer means, and the bring-up between a configured interface and a +//! disk with a size — as decisions with no transfer in them. +//! +//! One logical unit, READ(10)/WRITE(10) only, no MODE SENSE. **Everything a +//! device answers is checked and never believed**, and a refusal is by name: a +//! size this driver cannot address, a block size that does not divide the host +//! block, a peripheral that is not a disk. +//! +//! [`BringUp`] has [`crate::enumerate`]'s shape and for its reason: it is +//! driven to its end in place today, and a driver that gives a scheduler pass +//! back between acts drives the same order. + +/// Nanoseconds since boot. +pub type Nanos = u64; + +/// The block size everything above this driver is written in; a device whose +/// sectors do not divide it is refused, not approximated. +pub const HOST_BLOCK: u32 = 4096; + +/// READ(10)'s operation code (SBC-3 §5.11). +pub const READ_10: u8 = 0x28; +/// WRITE(10)'s operation code (SBC-3 §5.32). +pub const WRITE_10: u8 = 0x2A; +/// INQUIRY's operation code (SPC-4 §6.4). +pub const INQUIRY: u8 = 0x12; + +/// One command descriptor block, with the direction of the data it moves. +/// +/// Built only here, so no command can name a length a CBW cannot carry or a +/// direction that is not its own. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub struct Cdb { + bytes: [u8; 16], + len: u8, + data_in: bool, +} + +impl Cdb { + const fn of(cdb: [u8; N], data_in: bool) -> Self { + let mut bytes = [0u8; 16]; + let mut at = 0; + while at < N { + bytes[at] = cdb[at]; + at += 1; + } + Self { bytes, len: N as u8, data_in } + } + + /// No data phase (SPC-4 §6.37). + pub const TEST_UNIT_READY: Self = Self::of([0x00; 6], false); + /// Fixed-format sense, 18 bytes allocated (SPC-4 §6.29). + pub const REQUEST_SENSE: Self = Self::of([0x03, 0, 0, 0, SENSE_BYTES as u8, 0], true); + /// The whole medium, LBA 0 and count 0 — all a block-device flush can mean + /// (SBC-3 §5.24). + pub const SYNCHRONIZE_CACHE: Self = Self::of([0x35, 0, 0, 0, 0, 0, 0, 0, 0, 0], false); + + /// READ(10) or WRITE(10) of `sectors` at `lba`. + pub fn transfer(write: bool, lba: u32, sectors: u16) -> Self { + let [a, b, c, d] = lba.to_be_bytes(); + let [hi, lo] = sectors.to_be_bytes(); + let opcode = if write { WRITE_10 } else { READ_10 }; + Self::of([opcode, 0, a, b, c, d, 0, hi, lo, 0], !write) + } + + /// The bytes that go into the CBW. + pub fn bytes(&self) -> &[u8] { + &self.bytes[..usize::from(self.len)] + } + + pub fn opcode(&self) -> u8 { + self.bytes[0] + } + + /// Whether the data phase, if the command has one, is device to host. + pub fn data_in(&self) -> bool { + self.data_in + } +} + +/// Bytes a REQUEST SENSE asks for. +pub const SENSE_BYTES: usize = 18; + +/// The sense key, ASC and ASCQ of fixed-format sense data (SPC-4 §4.5.3). +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub struct Sense { + pub key: u8, + pub asc: u8, + pub ascq: u8, +} + +impl Sense { + /// What a device that would not say is taken to have said: zero is the + /// failing side of every decision made from it. + pub const NONE: Self = Self { key: 0, asc: 0, ascq: 0 }; + + /// The sense `response` carries, of which `delivered` bytes arrived. ASCQ + /// is byte 13, so fourteen must have or none of it is believed. + pub fn of(response: &[u8; SENSE_BYTES], delivered: u32) -> Self { + if delivered < 14 { + return Self::NONE; + } + Self { key: response[2] & 0x0F, asc: response[12], ascq: response[13] } + } + + /// ILLEGAL REQUEST / INVALID COMMAND OPERATION CODE: an answer, not a + /// failure, for a command SBC makes optional. + pub fn unimplemented(self) -> bool { + (self.key, self.asc, self.ascq) == (0x05, 0x20, 0x00) + } +} + +impl core::fmt::Display for Sense { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + write!(f, "{:#04x}/{:#04x}/{:#04x}", self.key, self.asc, self.ascq) + } +} + +/// One SCSI command, after the transport's own recovery. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum Outcome { + Ok { delivered: u32 }, + /// Understood and declined: an optional command's caller must tell "I will + /// not" from "I cannot". + Refused(Sense), + /// The transport broke, or the device contradicted itself; nothing about + /// the buffer is known. + Broken, + /// Not issued: the caller's budget was spent. Not a fact about the disk. + Budget, +} + +/// Why an operation failed, for the caller above the disk. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum Fail { + Device, + /// Nothing is known to have reached the device: ask again. + Budget, +} + +/// What a disk is, from its READ CAPACITY. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub struct Geometry { + pub sector_bytes: u32, + pub sectors: u64, + pub sectors_per_block: u32, + /// Whole [`HOST_BLOCK`]s. + pub blocks: u64, +} + +impl Geometry { + /// A disk not yet asked its size, which no transfer fits. + pub const NONE: Self = Self { sector_bytes: 0, sectors: 0, sectors_per_block: 0, blocks: 0 }; +} + +/// A caller's transfer that runs past the disk. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub struct PastTheEnd { + pub lba: u64, + pub count: u32, + pub blocks: u64, +} + +/// `count` host blocks at `lba`, as the READ(10) or WRITE(10) commands that +/// move them, at most `most` blocks each. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub struct Transfer { + lba: u64, + count: u32, + done: u32, + write: bool, + sectors_per_block: u32, + most: u32, +} + +/// One command of a [`Transfer`]. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub struct Batch { + pub cdb: Cdb, + /// Host blocks it moves, and the bytes. + pub blocks: u32, + pub bytes: usize, + /// Where in the caller's buffer they are. + pub offset: usize, + /// The host block it starts at. + pub block: u64, +} + +/// What one [`Batch`] came to. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum Moved { + Whole, + /// Short of what was asked: nothing above can say which blocks arrived, + /// so the transfer failed. + Short { delivered: u32 }, + Refused(Sense), + Ended(Fail), +} + +impl Moved { + /// Whether the device reported the batch complete, whole or in part. + pub fn reported(self) -> bool { + matches!(self, Self::Whole | Self::Short { .. }) + } +} + +impl Transfer { + /// A transfer on a disk of `geometry`; nothing to do for `count` zero. + pub fn new(lba: u64, count: u32, write: bool, geometry: &Geometry, most: u32) -> Result { + let sectors = u64::from(most) * u64::from(geometry.sectors_per_block); + assert!(most > 0 && u16::try_from(sectors).is_ok(), "a batch READ(10) cannot count"); + let fits = lba.checked_add(u64::from(count)).is_some_and(|end| end <= geometry.blocks); + if count > 0 && !fits { + return Err(PastTheEnd { lba, count, blocks: geometry.blocks }); + } + Ok(Self { lba, count, done: 0, write, sectors_per_block: geometry.sectors_per_block, most }) + } + + /// The command owed next, or `None` once every block has moved. + pub fn next(&self) -> Option { + if self.done == self.count { + return None; + } + let blocks = (self.count - self.done).min(self.most); + let block = self.lba + u64::from(self.done); + let sector = block * u64::from(self.sectors_per_block); + // `BringUp` refused a disk whose last sector does not fit 32 bits, and + // `new` refused a transfer past the disk. + let sector = u32::try_from(sector).expect("a sector past what READ(10) addresses"); + let sectors = (blocks * self.sectors_per_block) as u16; + Some(Batch { + cdb: Cdb::transfer(self.write, sector, sectors), + blocks, + bytes: blocks as usize * HOST_BLOCK as usize, + offset: self.done as usize * HOST_BLOCK as usize, + block, + }) + } + + /// What `outcome` of `batch` comes to. **Only the first batch may answer + /// "ask again"**: blocks already moved are on the device with no way to + /// resume. + pub fn answered(&mut self, batch: &Batch, outcome: Outcome) -> Moved { + let first = self.done == 0; + match outcome { + Outcome::Ok { delivered } if delivered as usize == batch.bytes => { + self.done += batch.blocks; + Moved::Whole + } + Outcome::Ok { delivered } => Moved::Short { delivered }, + Outcome::Refused(sense) => Moved::Refused(sense), + Outcome::Budget if first => Moved::Ended(Fail::Budget), + Outcome::Broken | Outcome::Budget => Moved::Ended(Fail::Device), + } + } +} + +/// What a SYNCHRONIZE CACHE came to. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum Flushed { + /// A cache was emptied. + Emptied, + /// The device has no cache to empty (INVALID COMMAND OPERATION CODE): its + /// writes are durable once complete, and a flush reports nothing wrong. + NoCache, + Refused(Sense), + Ended(Fail), +} + +pub fn flushed(outcome: Outcome) -> Flushed { + match outcome { + Outcome::Refused(sense) if sense.unimplemented() => Flushed::NoCache, + Outcome::Ok { .. } => Flushed::Emptied, + Outcome::Refused(sense) => Flushed::Refused(sense), + Outcome::Broken => Flushed::Ended(Fail::Device), + Outcome::Budget => Flushed::Ended(Fail::Budget), + } +} + +/// A question the bring-up reads an answer to. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum Query { + Inquiry, + Capacity10, + Capacity16, +} + +impl Query { + pub fn cdb(self) -> Cdb { + match self { + Self::Inquiry => Cdb::of([INQUIRY, 0, 0, 0, 36, 0], true), + Self::Capacity10 => Cdb::of([0x25, 0, 0, 0, 0, 0, 0, 0, 0, 0], true), + // SERVICE ACTION IN(16) / READ CAPACITY(16), 32 bytes allocated. + Self::Capacity16 => Cdb::of([0x9E, 0x10, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 32, 0, 0], true), + } + } + + /// The data phase's length: what the CDB allocates. + pub fn allocation(self) -> usize { + match self { + Self::Inquiry => 36, + Self::Capacity10 => 8, + Self::Capacity16 => 32, + } + } + + /// Bytes that must arrive for the answer to be read at all. + fn needs(self) -> usize { + match self { + Self::Inquiry => 36, + Self::Capacity10 => 8, + Self::Capacity16 => 12, + } + } + + pub fn named(self) -> &'static str { + match self { + Self::Inquiry => "INQUIRY", + Self::Capacity10 => "READ CAPACITY(10)", + Self::Capacity16 => "READ CAPACITY(16)", + } + } +} + +/// INQUIRY's vendor, product and revision (SPC-4 §6.4.2), bytes 8 to 35. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub struct Inquiry(pub [u8; 28]); + +impl Inquiry { + pub fn vendor(&self) -> &[u8] { + &self.0[..8] + } + + pub fn product(&self) -> &[u8] { + &self.0[8..24] + } +} + +/// What the bring-up asks the driver for next. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum Ask { + /// A TEST UNIT READY, on the transport directly: NOT READY is expected and + /// is not a failure to recover from. + TestUnitReady, + RequestSense, + /// Climb the recovery ladder for the TEST UNIT READY that broke. + Recover, + /// This question, with the transport's recovery behind it. + Read(Query), +} + +/// What the driver heard back from the last [`Ask`]. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum Heard<'a> { + /// CSW status 0. + Good, + /// CSW status 1: the device holds sense for whoever asks next. + CheckCondition, + /// The round trip broke. + Broke, + Sense(Sense), + /// `offline`: the ladder took the device offline. + Recovered { offline: bool }, + /// The allocation's bytes and how many of them arrived. + Data { bytes: &'a [u8], delivered: u32 }, + /// Refused, broken, or not issued. + Unanswered, +} + +/// How a bring-up ended. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum Up { + Ready(Geometry), + /// It never said it was ready: the sense it last gave, and whether the + /// ladder took it offline meanwhile. One that is not offline is a device a + /// later enumeration may find ready. + Unready { sense: Sense, offline: bool }, + Refused(Refusal), +} + +/// Why a device that answered is not a disk this driver serves. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum Refusal { + Unanswered(Query), + /// INQUIRY's peripheral device type, not 0 (direct access). + NotADisk(u8), + /// A sector size that does not divide [`HOST_BLOCK`]; zero among them. + SectorSize(u32), + /// A last LBA past what READ(10)'s 32 bits address: serving the first + /// 2 TiB of a bigger disk would silently truncate it. + PastRead10 { last_lba: u64 }, + LessThanABlock { sectors: u64, sector_bytes: u32 }, +} + +/// Where a bring-up stands. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +enum At { + Ready, + Sense, + Recovering, + Read(Query), +} + +/// TEST UNIT READY on a budget, then INQUIRY, then READ CAPACITY(10) and (16) +/// where the 10-byte form cannot say: everything between a configured +/// interface and a disk with a size. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub struct BringUp { + at: At, + /// When no further TEST UNIT READY is started; the one running finishes. + give_up: Nanos, + /// The last sense the device gave while not ready. + sense: Sense, +} + +/// Where a bring-up goes after an answer. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum Next { + Ask(BringUp, Ask), + /// INQUIRY named a direct-access device: this is what it said, and this is + /// asked next. + Disk(BringUp, Inquiry, Ask), + Up(Up), +} + +impl BringUp { + /// A bring-up that stops starting ready attempts at `give_up`. + pub fn begin(give_up: Nanos) -> (Self, Ask) { + (Self { at: At::Ready, give_up, sense: Sense::NONE }, Ask::TestUnitReady) + } + + fn ask(self, at: At, ask: Ask) -> Next { + Next::Ask(Self { at, ..self }, ask) + } + + /// Another attempt, if the budget has room for one at `now`. + fn again(self, now: Nanos, offline: bool) -> Next { + if offline || now >= self.give_up { + return Next::Up(Up::Unready { sense: self.sense, offline }); + } + self.ask(At::Ready, Ask::TestUnitReady) + } + + /// The last ask was answered with `heard`, at `now`. + pub fn heard(mut self, heard: Heard<'_>, now: Nanos) -> Next { + match (self.at, heard) { + (At::Ready, Heard::Good) => self.ask(At::Read(Query::Inquiry), Ask::Read(Query::Inquiry)), + // Fetching the sense also clears the condition on a device still + // spinning up. + (At::Ready, Heard::CheckCondition) => self.ask(At::Sense, Ask::RequestSense), + (At::Ready, Heard::Broke) => self.ask(At::Recovering, Ask::Recover), + (At::Sense, Heard::Sense(sense)) => { + self.sense = sense; + self.again(now, false) + } + (At::Recovering, Heard::Recovered { offline }) => self.again(now, offline), + (At::Read(query), Heard::Unanswered) => Next::Up(Up::Refused(Refusal::Unanswered(query))), + (At::Read(query), Heard::Data { bytes, delivered }) => { + assert_eq!(bytes.len(), query.allocation(), "the allocation is what is handed back"); + if (delivered as usize) < query.needs() { + return Next::Up(Up::Refused(Refusal::Unanswered(query))); + } + self.read(query, bytes) + } + (at, heard) => panic!("bring-up at {at:?} was answered {heard:?}, which it did not ask for"), + } + } + + fn read(self, query: Query, bytes: &[u8]) -> Next { + let be32 = |at: usize| u32::from_be_bytes([bytes[at], bytes[at + 1], bytes[at + 2], bytes[at + 3]]); + match query { + Query::Inquiry => { + let peripheral = bytes[0] & 0x1F; + if peripheral != 0 { + return Next::Up(Up::Refused(Refusal::NotADisk(peripheral))); + } + let mut said = [0u8; 28]; + said.copy_from_slice(&bytes[8..36]); + let next = Query::Capacity10; + Next::Disk(Self { at: At::Read(next), ..self }, Inquiry(said), Ask::Read(next)) + } + // An all-ones last LBA says the disk needs the 16-byte form. + Query::Capacity10 if be32(0) == u32::MAX => { + self.ask(At::Read(Query::Capacity16), Ask::Read(Query::Capacity16)) + } + Query::Capacity10 => Next::Up(geometry(u64::from(be32(0)), be32(4))), + Query::Capacity16 => { + let last = (u64::from(be32(0)) << 32) | u64::from(be32(4)); + Next::Up(geometry(last, be32(8))) + } + } + } +} + +/// The disk a READ CAPACITY describes, or why it is not one this driver serves. +fn geometry(last_lba: u64, sector_bytes: u32) -> Up { + if !matches!(sector_bytes, 512 | 1024 | 2048 | 4096) { + return Up::Refused(Refusal::SectorSize(sector_bytes)); + } + if last_lba > u64::from(u32::MAX) { + return Up::Refused(Refusal::PastRead10 { last_lba }); + } + let sectors = last_lba + 1; + let sectors_per_block = HOST_BLOCK / sector_bytes; + let blocks = sectors / u64::from(sectors_per_block); + if blocks == 0 { + return Up::Refused(Refusal::LessThanABlock { sectors, sector_bytes }); + } + Up::Ready(Geometry { sector_bytes, sectors, sectors_per_block, blocks }) +} + +/// A device-supplied ASCII field, rendered without letting it choose what the +/// log looks like. +pub struct Printable<'a>(pub &'a [u8]); + +impl core::fmt::Display for Printable<'_> { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + f.write_str("\"")?; + let mut utf8 = [0u8; 4]; + for &b in self.0 { + let c = if (0x20..0x7F).contains(&b) && b != b'"' { b as char } else { '.' }; + f.write_str(c.encode_utf8(&mut utf8))?; + } + f.write_str("\"") + } +} + +#[cfg(test)] +mod tests { + use super::*; + + const MS: Nanos = 1_000_000; + + /// SBC-3 §5.11 and §5.32: the operation code, the LBA big endian in bytes + /// 2 to 5, the transfer length big endian in 7 and 8. + #[test] + fn a_transfer_cdb_is_laid_out_as_sbc_defines_it() { + let read = Cdb::transfer(false, 0x0102_0304, 0x0506); + assert_eq!(read.bytes(), [0x28, 0, 1, 2, 3, 4, 0, 5, 6, 0]); + assert!(read.data_in()); + let write = Cdb::transfer(true, 0xFFFF_FFFF, 1); + assert_eq!(write.bytes(), [0x2A, 0, 0xFF, 0xFF, 0xFF, 0xFF, 0, 0, 1, 0]); + assert!(!write.data_in()); + } + + #[test] + fn every_fixed_cdb_carries_its_own_length_direction_and_allocation() { + assert_eq!(Cdb::TEST_UNIT_READY.bytes(), [0; 6]); + assert_eq!(Cdb::REQUEST_SENSE.bytes(), [0x03, 0, 0, 0, 18, 0]); + assert_eq!(Cdb::SYNCHRONIZE_CACHE.bytes(), [0x35, 0, 0, 0, 0, 0, 0, 0, 0, 0]); + assert!(!Cdb::TEST_UNIT_READY.data_in() && !Cdb::SYNCHRONIZE_CACHE.data_in()); + assert!(Cdb::REQUEST_SENSE.data_in()); + assert_eq!(Query::Inquiry.cdb().bytes(), [0x12, 0, 0, 0, 36, 0]); + assert_eq!(Query::Capacity10.cdb().bytes(), [0x25, 0, 0, 0, 0, 0, 0, 0, 0, 0]); + assert_eq!(Query::Capacity16.cdb().bytes(), [0x9E, 0x10, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 32, 0, 0]); + for query in [Query::Inquiry, Query::Capacity10, Query::Capacity16] { + assert!(query.cdb().data_in(), "{query:?}"); + assert!(query.needs() <= query.allocation(), "{query:?}"); + } + // The allocation length the CDB states is the data phase asked for. + assert_eq!(usize::from(Query::Inquiry.cdb().bytes()[4]), Query::Inquiry.allocation()); + assert_eq!(usize::from(Query::Capacity16.cdb().bytes()[13]), Query::Capacity16.allocation()); + } + + #[test] + fn sense_is_believed_only_when_its_ascq_arrived() { + let mut response = [0u8; SENSE_BYTES]; + response[2] = 0xF5; + response[12] = 0x20; + response[13] = 0x00; + assert_eq!(Sense::of(&response, 14), Sense { key: 0x05, asc: 0x20, ascq: 0 }); + assert!(Sense::of(&response, 18).unimplemented()); + assert_eq!(Sense::of(&response, 13), Sense::NONE); + assert!(!Sense::NONE.unimplemented()); + assert!(!Sense { key: 0x04, asc: 0x44, ascq: 0 }.unimplemented()); + extern crate std; + use std::string::ToString; + assert_eq!(Sense { key: 0x04, asc: 0x44, ascq: 0 }.to_string(), "0x04/0x44/0x00"); + } + + const STICK: Geometry = Geometry { sector_bytes: 512, sectors: 160, sectors_per_block: 8, blocks: 20 }; + + #[test] + fn a_transfer_past_the_disk_is_refused_and_an_empty_one_is_nothing() { + let past = |lba, count| Transfer::new(lba, count, false, &STICK, 8); + assert_eq!(past(19, 2), Err(PastTheEnd { lba: 19, count: 2, blocks: 20 })); + assert_eq!(past(u64::MAX, 1).map(|_| ()), Err(PastTheEnd { lba: u64::MAX, count: 1, blocks: 20 })); + assert!(past(19, 1).is_ok()); + let empty = past(u64::MAX, 0).expect("nothing to move"); + assert_eq!(empty.next(), None); + } + + /// Batches of at most `most` blocks, each at its own sector and offset, + /// and a batch that did not move whole is not moved past. + #[test] + fn a_transfer_is_its_batches_in_order_and_only_a_whole_one_advances() { + let mut transfer = Transfer::new(2, 18, true, &STICK, 8).expect("fits"); + let mut seen = [None; 4]; + for slot in &mut seen { + let Some(batch) = transfer.next() else { break }; + *slot = Some((batch.cdb, batch.blocks, batch.offset, batch.block)); + let bytes = batch.bytes as u32; + assert_eq!(transfer.answered(&batch, Outcome::Ok { delivered: bytes - 1 }), Moved::Short { delivered: bytes - 1 }); + assert_eq!(transfer.next(), Some(batch), "a short batch is not moved past"); + assert_eq!(transfer.answered(&batch, Outcome::Ok { delivered: bytes }), Moved::Whole); + } + assert_eq!(seen, [ + Some((Cdb::transfer(true, 16, 64), 8, 0, 2)), + Some((Cdb::transfer(true, 80, 64), 8, 8 * 4096, 10)), + Some((Cdb::transfer(true, 144, 16), 2, 16 * 4096, 18)), + None, + ]); + } + + /// "Ask again" says nothing reached the device, which is true only before + /// the first batch moved. + #[test] + fn only_the_first_batch_may_answer_ask_again() { + let mut transfer = Transfer::new(0, 16, false, &STICK, 8).expect("fits"); + let first = transfer.next().expect("a batch"); + assert_eq!(transfer.answered(&first, Outcome::Budget), Moved::Ended(Fail::Budget)); + assert_eq!(transfer.answered(&first, Outcome::Broken), Moved::Ended(Fail::Device)); + let sense = Sense { key: 3, asc: 0x11, ascq: 0 }; + assert_eq!(transfer.answered(&first, Outcome::Refused(sense)), Moved::Refused(sense)); + assert_eq!(transfer.answered(&first, Outcome::Ok { delivered: 8 * 4096 }), Moved::Whole); + let second = transfer.next().expect("a batch"); + assert_eq!(transfer.answered(&second, Outcome::Budget), Moved::Ended(Fail::Device)); + assert!(Moved::Whole.reported() && Moved::Short { delivered: 1 }.reported()); + assert!(!Moved::Refused(sense).reported() && !Moved::Ended(Fail::Budget).reported()); + } + + #[test] + fn a_flush_the_device_does_not_implement_is_no_failure_and_every_other_refusal_is() { + let unimplemented = Sense { key: 0x05, asc: 0x20, ascq: 0 }; + let failed = Sense { key: 0x04, asc: 0x44, ascq: 0 }; + assert_eq!(flushed(Outcome::Refused(unimplemented)), Flushed::NoCache); + assert_eq!(flushed(Outcome::Refused(failed)), Flushed::Refused(failed)); + assert_eq!(flushed(Outcome::Ok { delivered: 0 }), Flushed::Emptied); + assert_eq!(flushed(Outcome::Broken), Flushed::Ended(Fail::Device)); + assert_eq!(flushed(Outcome::Budget), Flushed::Ended(Fail::Budget)); + } + + fn inquiry(peripheral: u8) -> [u8; 36] { + let mut data = [0u8; 36]; + data[0] = peripheral; + data[8..16].copy_from_slice(b"QEMU "); + data[16..32].copy_from_slice(b"QEMU HARDDISK "); + data[32..36].copy_from_slice(b"2.5+"); + data + } + + fn capacity10(last: u32, bytes: u32) -> [u8; 8] { + let mut data = [0u8; 8]; + data[..4].copy_from_slice(&last.to_be_bytes()); + data[4..].copy_from_slice(&bytes.to_be_bytes()); + data + } + + fn capacity16(last: u64, bytes: u32) -> [u8; 32] { + let mut data = [0u8; 32]; + data[..8].copy_from_slice(&last.to_be_bytes()); + data[8..12].copy_from_slice(&bytes.to_be_bytes()); + data + } + + /// A device that answers every ask as `answer` says; `None` is the ask + /// the test expects never to be made. + struct Device<'a> { + not_ready: u32, + breaks: u32, + offline: bool, + inquiry: Option<&'a [u8]>, + capacity10: Option<&'a [u8]>, + capacity16: Option<&'a [u8]>, + } + + const LONGEST: usize = 16; + + struct Route { + asks: [Option; LONGEST], + disk: Option, + up: Up, + } + + impl Route { + fn asks(&self) -> impl Iterator + '_ { + self.asks.iter().map_while(|a| *a) + } + } + + /// A bring-up driven to its end against `device`, the clock advancing + /// `step` per ask from 0 with a 500 ms budget. + fn bring_up(mut device: Device<'_>, step: Nanos) -> Route { + let (mut up, mut ask) = BringUp::begin(500 * MS); + let mut asks = [None; LONGEST]; + let mut disk = None; + let mut now = 0; + for slot in &mut asks { + *slot = Some(ask); + now += step; + let heard = match ask { + Ask::TestUnitReady if device.breaks > 0 => { + device.breaks -= 1; + Heard::Broke + } + Ask::TestUnitReady if device.not_ready > 0 => { + device.not_ready -= 1; + Heard::CheckCondition + } + Ask::TestUnitReady => Heard::Good, + Ask::RequestSense => Heard::Sense(Sense { key: 0x02, asc: 0x04, ascq: 0x01 }), + Ask::Recover => Heard::Recovered { offline: device.offline }, + Ask::Read(query) => { + let data = match query { + Query::Inquiry => device.inquiry, + Query::Capacity10 => device.capacity10, + Query::Capacity16 => device.capacity16, + }; + match data { + Some(bytes) => Heard::Data { bytes, delivered: bytes.len() as u32 }, + None => Heard::Unanswered, + } + } + }; + match up.heard(heard, now) { + Next::Ask(next, then) => (up, ask) = (next, then), + Next::Disk(next, said, then) => { + disk = Some(said); + (up, ask) = (next, then); + } + Next::Up(end) => return Route { asks, disk, up: end }, + } + } + panic!("a bring-up that does not end: {asks:?}"); + } + + fn disk<'a>(inquiry: &'a [u8], capacity10: &'a [u8]) -> Device<'a> { + Device { + not_ready: 0, + breaks: 0, + offline: false, + inquiry: Some(inquiry), + capacity10: Some(capacity10), + capacity16: None, + } + } + + #[test] + fn a_ready_disk_is_asked_what_it_is_and_how_big_and_nothing_else() { + let (inquiry, capacity) = (inquiry(0), capacity10(8191, 512)); + let route = bring_up(disk(&inquiry, &capacity), MS); + assert!(route.asks().eq([Ask::TestUnitReady, Ask::Read(Query::Inquiry), Ask::Read(Query::Capacity10)])); + assert_eq!(route.up, Up::Ready(Geometry { sector_bytes: 512, sectors: 8192, sectors_per_block: 8, blocks: 1024 })); + let said = route.disk.expect("INQUIRY said a disk"); + assert_eq!((said.vendor(), said.product()), (&b"QEMU "[..], &b"QEMU HARDDISK "[..])); + assert_eq!(&said.0[24..], b"2.5+"); + } + + /// Each NOT READY is followed by the sense that clears it, and the device + /// is asked again until it is ready. + #[test] + fn a_disk_spinning_up_is_asked_for_its_sense_and_then_again() { + let (inquiry, capacity) = (inquiry(0), capacity10(8191, 4096)); + let route = bring_up(Device { not_ready: 2, ..disk(&inquiry, &capacity) }, MS); + assert!(route.asks().take(5).eq([ + Ask::TestUnitReady, + Ask::RequestSense, + Ask::TestUnitReady, + Ask::RequestSense, + Ask::TestUnitReady, + ])); + assert!(matches!(route.up, Up::Ready(Geometry { sectors_per_block: 1, blocks: 8192, .. }))); + } + + /// The budget bounds when attempts stop being *started*: the sense of the + /// last one is what the refusal says. + #[test] + fn a_disk_that_never_becomes_ready_is_given_up_on_at_its_budget() { + let (inquiry, capacity) = (inquiry(0), capacity10(8191, 512)); + let route = bring_up(Device { not_ready: u32::MAX, ..disk(&inquiry, &capacity) }, 100 * MS); + assert_eq!(route.up, Up::Unready { sense: Sense { key: 0x02, asc: 0x04, ascq: 0x01 }, offline: false }); + assert_eq!(route.asks().filter(|a| *a == Ask::TestUnitReady).count(), 3, "at 0, 200 and 400 ms"); + assert_eq!(route.disk, None); + } + + #[test] + fn a_ready_attempt_that_breaks_is_recovered_and_one_taken_offline_ends_it() { + let (inquiry, capacity) = (inquiry(0), capacity10(8191, 512)); + let recovered = bring_up(Device { breaks: 1, ..disk(&inquiry, &capacity) }, MS); + assert!(recovered.asks().take(3).eq([Ask::TestUnitReady, Ask::Recover, Ask::TestUnitReady])); + assert!(matches!(recovered.up, Up::Ready(_))); + let offline = bring_up(Device { breaks: 1, offline: true, ..disk(&inquiry, &capacity) }, MS); + assert!(offline.asks().eq([Ask::TestUnitReady, Ask::Recover])); + assert_eq!(offline.up, Up::Unready { sense: Sense::NONE, offline: true }); + } + + /// READ CAPACITY(10) says all ones when the disk needs sixteen bytes to + /// describe it, and only then is the 16-byte form asked. + #[test] + fn a_disk_too_big_for_read_capacity_10_is_asked_the_16_byte_form() { + let (inquiry, ten) = (inquiry(0), capacity10(u32::MAX, 512)); + let sixteen = capacity16(u64::from(u32::MAX), 4096); + let route = bring_up(Device { capacity16: Some(&sixteen), ..disk(&inquiry, &ten) }, MS); + assert_eq!(route.asks().last(), Some(Ask::Read(Query::Capacity16))); + assert_eq!( + route.up, + Up::Ready(Geometry { sector_bytes: 4096, sectors: 1 << 32, sectors_per_block: 1, blocks: 1 << 32 }) + ); + let past = capacity16(u64::from(u32::MAX) + 1, 512); + let route = bring_up(Device { capacity16: Some(&past), ..disk(&inquiry, &ten) }, MS); + assert_eq!(route.up, Up::Refused(Refusal::PastRead10 { last_lba: 1 << 32 })); + } + + #[test] + fn a_device_that_is_not_a_disk_this_driver_serves_is_refused_by_name() { + let (disk_inquiry, fine) = (inquiry(0), capacity10(8191, 512)); + let cdrom = inquiry(0x05); + assert_eq!(bring_up(disk(&cdrom, &fine), MS).up, Up::Refused(Refusal::NotADisk(0x05))); + // The qualifier bits are not the type. + assert!(matches!(bring_up(disk(&inquiry(0x20), &fine), MS).up, Up::Ready(_))); + for bytes in [0, 520, 8192, 256] { + let odd = capacity10(8191, bytes); + assert_eq!(bring_up(disk(&disk_inquiry, &odd), MS).up, Up::Refused(Refusal::SectorSize(bytes))); + } + let tiny = capacity10(6, 512); + assert_eq!( + bring_up(disk(&disk_inquiry, &tiny), MS).up, + Up::Refused(Refusal::LessThanABlock { sectors: 7, sector_bytes: 512 }) + ); + } + + #[test] + fn a_question_unanswered_or_answered_short_refuses_the_device() { + let (whole, fine) = (inquiry(0), capacity10(8191, 512)); + let silent = Device { inquiry: None, ..disk(&whole, &fine) }; + assert_eq!(bring_up(silent, MS).up, Up::Refused(Refusal::Unanswered(Query::Inquiry))); + let no_size = Device { capacity10: None, ..disk(&whole, &fine) }; + assert_eq!(bring_up(no_size, MS).up, Up::Refused(Refusal::Unanswered(Query::Capacity10))); + + let (up, _) = BringUp::begin(500 * MS); + let Next::Ask(up, _) = up.heard(Heard::Good, 0) else { panic!("INQUIRY is asked") }; + let short = up.heard(Heard::Data { bytes: &whole, delivered: 35 }, 0); + assert_eq!(short, Next::Up(Up::Refused(Refusal::Unanswered(Query::Inquiry)))); + } + + #[test] + #[should_panic(expected = "which it did not ask for")] + fn an_answer_to_something_not_asked_is_a_driver_bug() { + let (up, _) = BringUp::begin(500 * MS); + let _ = up.heard(Heard::Sense(Sense::NONE), 0); + } + + #[test] + fn a_device_field_prints_as_the_characters_it_carries_and_nothing_else() { + extern crate std; + use std::string::ToString; + assert_eq!(Printable(b"AB\"1\x7f\n z").to_string(), "\"AB.1.. z\""); + } +} From f90b2d2243415e7b1f9beed4e3c6cf40417988ad Mon Sep 17 00:00:00 2001 From: japabu Date: Mon, 28 Sep 2026 23:04:27 +0200 Subject: [PATCH 2/8] Round 1 review fixes: one CC_STALL, the data-stall walks, the port-gone seam exercised MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - CC_STALL lives in toyos_xhci::job beside CC_SUCCESS and CC_SHORT_PACKET; the kernel's EP0 recovery imports it and its own copy is deleted. - bot: two walks on the data-stall route. A Data STALL leaving 100 B unmoved then CSW (TAG, 0, 0) delivers 412 B; a Data STALL, a Status STALL and a whole CSW asks the status twice and ends Ok. A controller residue larger than the transfer delivers nothing, on both routes. - bot: the CSW is judged in BOT 1.0 §6.3.2's order: status 2 is a phase error whatever the residue, 0 and 1 are refused for a residue past the transfer, and 3..=255 are Broke::Reserved, no longer a phase error. - msc.rs: the staged PortGone is the Command act's completion, completed(Err(Quiet::Gone)), so the actuator goes through the arm that sends a disconnect to the teardown rather than past it. - scsi: the crate's Nanos, not a fifth alias; Geometry's fields are private behind accessors, and a test pins that Geometry::NONE fits no transfer; Outcome is Reply, apart from the crate root's job::Outcome; the refusal of 256-byte sectors is stated as this driver's set, not SBC-3's. - Removed: msc.rs's "every decision is the crate's" paragraph, scsi.rs's "driven in place today" paragraph, SectorSize's doc and Broke::Csw's doc. Mutations, each a checked patch built, run and reversed (cargo test -p toyos-xhci EXIT=101 on each): stall moved = data_len; stall status(true); data residue wrapping_sub; the CSW's old residue-first order; stall residue wrapping_sub. Co-Authored-By: Claude Opus 5.5 --- kernel/src/drivers/xhci/mod.rs | 1 - kernel/src/drivers/xhci/wait/mod.rs | 4 +- kernel/src/drivers/xhci/wait/msc.rs | 83 ++++++++++++----------- toyos-xhci/src/bot.rs | 82 ++++++++++++++++++----- toyos-xhci/src/job.rs | 2 + toyos-xhci/src/scsi.rs | 100 ++++++++++++++++------------ 6 files changed, 168 insertions(+), 104 deletions(-) diff --git a/kernel/src/drivers/xhci/mod.rs b/kernel/src/drivers/xhci/mod.rs index ef82aa6be0..4f202204c4 100644 --- a/kernel/src/drivers/xhci/mod.rs +++ b/kernel/src/drivers/xhci/mod.rs @@ -113,7 +113,6 @@ const EVENT_PORT_STATUS_CHANGE: u32 = 34; // CC_SHORT_PACKET is success with a residue, not an error — treating it as one is the classic mass-storage bug. const CC_SUCCESS: u32 = 1; -const CC_STALL: u32 = 6; const CC_SHORT_PACKET: u32 = 13; const CC_CONTEXT_STATE_ERROR: u32 = 19; /// Stopped, Stopped - Length Invalid and Stopped - Short Packet (Table 6-90): the transfer events a Stop Endpoint raises for the TRB it stopped inside (§4.6.9). diff --git a/kernel/src/drivers/xhci/wait/mod.rs b/kernel/src/drivers/xhci/wait/mod.rs index 1c81b549cd..97d9ce6d7f 100644 --- a/kernel/src/drivers/xhci/wait/mod.rs +++ b/kernel/src/drivers/xhci/wait/mod.rs @@ -41,7 +41,7 @@ use super::{deadline, enqueue_control, log_unrecoverable, Completion, Trb, TrbRi use super::{XhciController, EVENT_TRANSFER, EVENT_CMD_COMPLETE, USB_TIMEOUT_NS}; use super::{CC_SUCCESS, CC_SHORT_PACKET}; use toyos_xhci::call::NotTaken; -use toyos_xhci::job::Await; +use toyos_xhci::job::{Await, CC_STALL}; use toyos_xhci::recovery::{Act, NeedsConfigure, Recovery}; use toyos_xhci::scan; @@ -482,7 +482,7 @@ impl XhciController { /// Take EP0 back out of Halted where `code` says the device stalled the /// transfer, before the failure reaches a caller likely to send another one. fn recover_after(&mut self, slot: u8, ctx_block: usize, ring: &mut TrbRing, code: u32) { - if code != super::CC_STALL { + if code != CC_STALL { return; } if !self.restart_control_endpoint(slot, ctx_block, ring) { diff --git a/kernel/src/drivers/xhci/wait/msc.rs b/kernel/src/drivers/xhci/wait/msc.rs index 99111be478..8b2500f652 100644 --- a/kernel/src/drivers/xhci/wait/msc.rs +++ b/kernel/src/drivers/xhci/wait/msc.rs @@ -4,11 +4,6 @@ //! controller: `with_disk` holds the controller lock for the whole of it. //! Everything here comes off the wire and is checked, never trusted; refusal //! is by name, never a panic. -//! -//! **Every decision is the crate's**: the round trip is -//! `toyos_xhci::bot::RoundTrip`, the SCSI above it `toyos_xhci::scsi`, the -//! recovery `toyos_xhci::ladder`. This module does the transfers they ask for, -//! waits for them in place, and says what happened. use crate::mm::Dma; @@ -34,7 +29,7 @@ use toyos_xhci::identity::{self, Identity, Serial, UsbId}; use toyos_xhci::ladder::{self, AfterReset, Left, PortStep, Run, Rung}; use toyos_xhci::port; use toyos_xhci::reset_recovery::{self, Answered, GaveUp, Look, Pipe, Quiescing, SlotGoes, Step}; -use toyos_xhci::scsi::{self, BringUp, Cdb, Fail, Flushed, Geometry, Heard, Moved, Outcome, Printable}; +use toyos_xhci::scsi::{self, BringUp, Cdb, Fail, Flushed, Geometry, Heard, Moved, Printable, Reply}; use toyos_xhci::scsi::{Refusal, Sense, Transfer, HOST_BLOCK}; /// A region, not an address: the CBW's length is the region's own size, so @@ -190,8 +185,8 @@ impl MscDevice { pub fn geometry(&self) -> StorageGeometry { StorageGeometry { - logical_block_bytes: self.geometry.sector_bytes, - blocks: self.geometry.blocks, + logical_block_bytes: self.geometry.sector_bytes(), + blocks: self.geometry.blocks(), } } @@ -271,6 +266,9 @@ impl core::fmt::Display for Told<'_> { write!(f, "the {phase} phase stalled and the endpoint reset did not clear it") } Broke::PhaseError => f.write_str("the device reported a phase error"), + Broke::Reserved { status } => { + write!(f, "the CSW carries status {status:#04x}, which the class reserves") + } Broke::Csw { what, got, want, status, residue } => write!( f, "CSW {what} {got:#x}, not {want:#x} (status {status}, {residue} B unmoved)" @@ -883,8 +881,8 @@ impl XhciController { } let cdb = Cdb::SYNCHRONIZE_CACHE; let issued = ctrl.scsi(dev, &cdb, None, until); - let outcome = flush_sense().map_or(issued, Outcome::Refused); - match scsi::flushed(outcome) { + let reply = flush_sense().map_or(issued, Reply::Refused); + match scsi::flushed(reply) { Flushed::NoCache => { dev.no_write_cache = true; log!("usb-storage: disk {number} does not implement SYNCHRONIZE CACHE \ @@ -941,8 +939,8 @@ impl XhciController { if let Host::From(src) = &host { dma.copy_from(dev.block + MSC_DATA, &src[offset..offset + bytes]); } - let outcome = self.scsi(dev, &batch.cdb, Some(data.subview(0, bytes)), until); - let moved = transfer.answered(&batch, outcome); + let reply = self.scsi(dev, &batch.cdb, Some(data.subview(0, bytes)), until); + let moved = transfer.answered(&batch, reply); if moved.reported() { dev.wrote(write); } @@ -984,7 +982,7 @@ impl XhciController { /// by whoever the call is — [`served`] for a block operation, the bind for /// each of its commands — so a later command of the same operation spends /// what the break left it, and no call inherits another's. - fn scsi(&mut self, dev: &mut MscDevice, cdb: &Cdb, data: DataPhase, until: Deadline) -> Outcome { + fn scsi(&mut self, dev: &mut MscDevice, cdb: &Cdb, data: DataPhase, until: Deadline) -> Reply { let opcode = cdb.opcode(); let data_out = data.is_some() && !cdb.data_in(); // Named per line so a multi-disk boot's retry log attributes to the @@ -1000,24 +998,24 @@ impl XhciController { Err(NotIssued::Operation) => { log!("usb-storage: {slot} SCSI {opcode:#04x} not issued: {}", crate::block::OPERATION); - return Outcome::Budget; + return Reply::Budget; } // A command re-issued with nothing left would have every wait // cut at once, and count against the device a break that was // the budget's. Err(NotIssued::Call(why)) => { log!("usb-storage: {slot} SCSI {opcode:#04x} not issued again: {why}"); - return Outcome::Budget; + return Reply::Budget; } } match self.bot(dev, cdb, data, Asks::Command) { Ok(Bot::Done { delivered }) => { self.transport_came_back(dev, opcode); - return Outcome::Ok { delivered }; + return Reply::Ok { delivered }; } Ok(Bot::Failed) => { self.transport_came_back(dev, opcode); - return Outcome::Refused(self.request_sense(dev)); + return Reply::Refused(self.request_sense(dev)); } // Not a transport that broke: a device that is no longer on // the bus. Its port's own teardown gives the slot and the pool @@ -1032,7 +1030,7 @@ impl XhciController { // A hold for its device is part of this call, from the // wait that saw it go. self.after_break.open(self.bulk_began, AFTER_BREAK); - return Outcome::Broken; + return Reply::Broken; } Err(why) => { self.after_break.open(self.bulk_began, AFTER_BREAK); @@ -1040,7 +1038,7 @@ impl XhciController { log!("usb-storage: {slot} transport broke on SCSI {opcode:#04x}: {broke}; \ break {} of {MAX_TRANSPORT_BREAKS} running", dev.run.breaks().saturating_add(1)); if !self.climb_until_in_step(dev, why.event(), why.left(data_out)) { - return Outcome::Broken; + return Reply::Broken; } } } @@ -1371,10 +1369,6 @@ impl XhciController { #[cfg(not(feature = "boot-actuators"))] let _ = asks; #[cfg(feature = "boot-actuators")] - if staged == Some(staged::Fault::PortGone) { - return Err(Broke::Gone { phase: Phase::Command }); - } - #[cfg(feature = "boot-actuators")] if staged == Some(staged::Fault::Unanswered) { let began = crate::clock::nanos_since_boot(); // The staged wait is the wait a break opens the call from. @@ -1418,14 +1412,19 @@ impl XhciController { let answer = match act { bot::Act::Command => { #[cfg(feature = "boot-actuators")] - let withheld = staged == Some(staged::Fault::NoCbw); + let staged_answer = match staged { + Some(staged::Fault::NoCbw) => Some(bot::Answer::Moved { code: CC_SUCCESS, residue: 0 }), + Some(staged::Fault::PortGone) => Some(completed(Err(Quiet::Gone))), + _ => None, + }; #[cfg(not(feature = "boot-actuators"))] - let withheld = false; - if withheld { - bot::Answer::Moved { code: CC_SUCCESS, residue: 0 } - } else { - let cbw_phys = dma.device_addr() + (dev.block + MSC_CBW) as u64; - completed(self.bulk(dev, false, cbw_phys, CBW_LEN as u32, Phase::Command, &open)) + let staged_answer = None; + match staged_answer { + Some(answer) => answer, + None => { + let cbw_phys = dma.device_addr() + (dev.block + MSC_CBW) as u64; + completed(self.bulk(dev, false, cbw_phys, CBW_LEN as u32, Phase::Command, &open)) + } } } bot::Act::Data(pipe) => { @@ -1969,8 +1968,8 @@ pub(in crate::drivers::xhci) fn bind( log!("usb-storage: disk {index} came back on port {} slot {slot_id} as the same device \ (USB {:04x}:{:04x}, serial number {}, {} blocks of {} B), msc_block +{:#x}; its \ volume carries on{}", - u32::from(port_idx) + 1, usb.vendor, usb.product, dev.identity.serial, dev.geometry.blocks, - dev.geometry.sector_bytes, block, + u32::from(port_idx) + 1, usb.vendor, usb.product, dev.identity.serial, dev.geometry.blocks(), + dev.geometry.sector_bytes(), block, if owed { OWED_A_FLUSH } else { "" }); ctrl.msc[at].disk = Some(Disk { index, dev }); return Bind::Bound; @@ -1979,9 +1978,9 @@ pub(in crate::drivers::xhci) fn bind( log!( "usb-storage: disk {index} ready on slot {slot_id}, {} blocks of {} B \ ({} MiB), msc_block +{:#x}", - dev.geometry.blocks, - dev.geometry.sector_bytes, - dev.geometry.blocks * u64::from(HOST_BLOCK) / (1024 * 1024), + dev.geometry.blocks(), + dev.geometry.sector_bytes(), + dev.geometry.blocks() * u64::from(HOST_BLOCK) / (1024 * 1024), block ); ctrl.msc[at].disk = Some(Disk { index, dev }); @@ -2079,18 +2078,18 @@ fn bring_up(ctrl: &mut XhciController, dev: &mut MscDevice) -> Up { scratch.zero(); // `subview` refuses a command asking for more than the scratch // buffer holds. Each command of a bind is a call of its own. - let outcome = ctrl.scsi(dev, &cdb, Some(scratch.subview(0, len)), until); + let reply = ctrl.scsi(dev, &cdb, Some(scratch.subview(0, len)), until); ctrl.after_break = AfterBreak::CLOSED; - match outcome { - Outcome::Ok { delivered } => { + match reply { + Reply::Ok { delivered } => { dma.copy_to(dev.block + MSC_SCRATCH, &mut read[..len]); Heard::Data { bytes: &read[..len], delivered } } - Outcome::Refused(sense) => { + Reply::Refused(sense) => { log_refusal(&cdb, sense); Heard::Unanswered } - Outcome::Broken | Outcome::Budget => Heard::Unanswered, + Reply::Broken | Reply::Budget => Heard::Unanswered, } } }; @@ -2111,8 +2110,8 @@ fn bring_up(ctrl: &mut XhciController, dev: &mut MscDevice) -> Up { return match end { scsi::Up::Ready(geometry) => { dev.geometry = geometry; - dev.identity.sectors = geometry.sectors; - dev.identity.sector_bytes = geometry.sector_bytes; + dev.identity.sectors = geometry.sectors(); + dev.identity.sector_bytes = geometry.sector_bytes(); Up::Ready } scsi::Up::Unready { sense, offline } => { diff --git a/toyos-xhci/src/bot.rs b/toyos-xhci/src/bot.rs index 7a292a8a97..5c40d51923 100644 --- a/toyos-xhci/src/bot.rs +++ b/toyos-xhci/src/bot.rs @@ -17,7 +17,7 @@ //! touches a ring, so a driver that waits in place and one that gives the CPU //! back between transfers drive the same order. -use crate::job::{CC_SHORT_PACKET, CC_SUCCESS}; +use crate::job::{CC_SHORT_PACKET, CC_STALL, CC_SUCCESS}; use crate::ladder::{self, Left}; use crate::reset_recovery::Pipe; use crate::scsi::Cdb; @@ -104,9 +104,6 @@ impl core::fmt::Display for Phase { } } -/// Completion code 6 (xHCI 1.2 Table 6-90): the device STALLed the transfer. -pub const CC_STALL: u32 = 6; - pub const CBW_LEN: usize = 31; pub const CSW_LEN: usize = 13; const CBW_SIGNATURE: u32 = 0x4342_5355; @@ -155,9 +152,8 @@ pub enum Broke { Stall { phase: Phase }, /// CSW status 2, which leaves both endpoints Running. PhaseError, - /// The CSW named somebody else's transfer; the status and residue are the - /// rest of what the device said, and tell a status made for an abandoned - /// command from one made for nothing. + /// A CSW status §5.2 reserves, which is no meaningful CSW (§6.3.2). + Reserved { status: u8 }, Csw { what: &'static str, got: u32, want: u32, status: u8, residue: u32 }, /// More bytes claimed unmoved than the transfer had; believing it would /// underflow the byte count every caller uses. @@ -174,7 +170,9 @@ impl Broke { | Self::Gone { phase } | Self::Stall { phase } | Self::Short { phase, .. } => *phase, - Self::PhaseError | Self::Csw { .. } | Self::Residue { .. } => Phase::Status, + Self::PhaseError | Self::Reserved { .. } | Self::Csw { .. } | Self::Residue { .. } => { + Phase::Status + } }; ladder::left(phase, data_out) } @@ -340,7 +338,8 @@ pub struct CswDue { } impl CswDue { - /// Every field checked (§6.3), never believed. + /// Every field checked (§6.3), never believed. Status 2 is meaningful + /// whatever the residue; 0 and 1 only with one no larger than the transfer. pub fn judge(self, csw: &[u8; CSW_LEN]) -> Result> { let word = |at: usize| u32::from_le_bytes([csw[at], csw[at + 1], csw[at + 2], csw[at + 3]]); let (signature, tag, residue, status) = (word(0), word(4), word(8), csw[12]); @@ -352,15 +351,14 @@ impl CswDue { if tag != self.tag { return Err(Broke::Csw { what: "tag", got: tag, want: self.tag, status, residue }); } - if residue > self.data_len { - return Err(Broke::Residue { unmoved: residue, of: self.data_len }); - } match status { + 2 => Err(Broke::PhaseError), + 0 | 1 if residue > self.data_len => Err(Broke::Residue { unmoved: residue, of: self.data_len }), // Neither account is trusted alone: a caller may read only what // both the device and the controller say arrived. 0 => Ok(Bot::Done { delivered: self.moved.min(self.data_len - residue) }), 1 => Ok(Bot::Failed), - _ => Err(Broke::PhaseError), + status => Err(Broke::Reserved { status }), } } } @@ -526,6 +524,11 @@ mod tests { assert_eq!(walk(read(1), 512, short, csw(TAG, 0, 0)).end, Ok(Bot::Done { delivered: 412 })); assert_eq!(walk(read(1), 512, |_| WHOLE, csw(TAG, 200, 0)).end, Ok(Bot::Done { delivered: 312 })); assert_eq!(walk(read(1), 512, short, csw(TAG, 50, 0)).end, Ok(Bot::Done { delivered: 412 })); + let overrun = |act| match act { + Act::Data(_) => Answer::Moved { code: CC_SHORT_PACKET, residue: 600 }, + _ => WHOLE, + }; + assert_eq!(walk(read(1), 512, overrun, csw(TAG, 0, 0)).end, Ok(Bot::Done { delivered: 0 })); } /// §6.7.2: the device STALLs a data phase it will not finish, and the @@ -547,6 +550,46 @@ mod tests { assert_eq!(walk.end, Ok(Bot::Failed)); } + /// A stalled data phase's residue is the controller's account, and a + /// status after the restart is the device's: neither alone. + #[test] + fn a_stalled_data_phase_delivers_what_both_the_controller_and_the_device_say_arrived() { + let stalls_leaving = |unmoved| { + move |act| match act { + Act::Data(_) => Answer::Moved { code: CC_STALL, residue: unmoved }, + Act::Restart { .. } => Answer::Restarted(true), + _ => WHOLE, + } + }; + assert_eq!(walk(read(1), 512, stalls_leaving(100), csw(TAG, 0, 0)).end, Ok(Bot::Done { delivered: 412 })); + assert_eq!(walk(read(1), 512, stalls_leaving(600), csw(TAG, 0, 0)).end, Ok(Bot::Done { delivered: 0 })); + } + + /// §5.3.3's one status retry is the status phase's, whatever the data + /// phase before it needed. + #[test] + fn a_status_stalled_after_a_data_stall_is_still_asked_for_again() { + let mut status_stalls = 1; + let walk = walk(read(1), 512, |act| match act { + Act::Data(_) => Answer::Moved { code: CC_STALL, residue: 512 }, + Act::Status if status_stalls > 0 => { + status_stalls -= 1; + Answer::Moved { code: CC_STALL, residue: CSW_LEN as u32 } + } + Act::Restart { .. } => Answer::Restarted(true), + _ => WHOLE, + }, csw(TAG, 512, 1)); + assert!(walk.acts().eq([ + Act::Command, + Act::Data(Pipe::In), + Act::Restart { pipe: Pipe::In, then: Phase::Data }, + Act::Status, + Act::Restart { pipe: Pipe::In, then: Phase::StatusOwed }, + Act::Status, + ])); + assert_eq!(walk.end, Ok(Bot::Failed)); + } + #[test] fn a_data_phase_whose_pipe_will_not_restart_breaks_as_a_stall() { let stuck = |act| match act { @@ -635,8 +678,9 @@ mod tests { } } - /// §6.3: valid means the signature and the tag, meaningful means a status - /// the class defines and a residue no larger than the transfer. + /// §6.3.1: valid means the signature and the tag. §6.3.2: meaningful means + /// status 0 or 1 with a residue no larger than the transfer, or status 2 + /// whatever the residue; §5.2 reserves every other status. #[test] fn a_csw_is_believed_only_when_it_is_valid_and_meaningful() { let judged = |csw: [u8; CSW_LEN]| walk(read(1), 512, |_| WHOLE, csw).end; @@ -648,10 +692,13 @@ mod tests { Err(Broke::Csw { what: "tag", got: TAG + 1, want: TAG, status: 1, residue: 7 }) ); assert_eq!(judged(csw(TAG, 513, 0)), Err(Broke::Residue { unmoved: 513, of: 512 })); + assert_eq!(judged(csw(TAG, 513, 1)), Err(Broke::Residue { unmoved: 513, of: 512 })); assert_eq!(judged(csw(TAG, 512, 0)), Ok(Bot::Done { delivered: 0 })); assert_eq!(judged(csw(TAG, 0, 1)), Ok(Bot::Failed)); - for status in 2..=u8::MAX { - assert_eq!(judged(csw(TAG, 0, status)), Err(Broke::PhaseError), "status {status}"); + assert_eq!(judged(csw(TAG, 0, 2)), Err(Broke::PhaseError)); + assert_eq!(judged(csw(TAG, 513, 2)), Err(Broke::PhaseError)); + for status in 3..=u8::MAX { + assert_eq!(judged(csw(TAG, 0, status)), Err(Broke::Reserved { status }), "status {status}"); } } @@ -669,6 +716,7 @@ mod tests { assert_eq!(gone.event(), None); for status in [ Broke::::PhaseError, + Broke::Reserved { status: 3 }, Broke::Residue { unmoved: 2, of: 1 }, Broke::Csw { what: "tag", got: 0, want: 1, status: 0, residue: 0 }, Broke::Silence { phase: Phase::Status, why: Why }, diff --git a/toyos-xhci/src/job.rs b/toyos-xhci/src/job.rs index b05c3cd07a..91f331ff47 100644 --- a/toyos-xhci/src/job.rs +++ b/toyos-xhci/src/job.rs @@ -50,6 +50,8 @@ pub enum Await { /// controller still owes whatever stages are left. pub const CC_SUCCESS: u32 = 1; pub const CC_SHORT_PACKET: u32 = 13; +/// Table 6-90's Stall Error: the device STALLed the transfer. +pub const CC_STALL: u32 = 6; /// What the controller still owes one operation after the completion it was /// submitted on. diff --git a/toyos-xhci/src/scsi.rs b/toyos-xhci/src/scsi.rs index a7cd87c213..cd99a0663f 100644 --- a/toyos-xhci/src/scsi.rs +++ b/toyos-xhci/src/scsi.rs @@ -6,13 +6,8 @@ //! device answers is checked and never believed**, and a refusal is by name: a //! size this driver cannot address, a block size that does not divide the host //! block, a peripheral that is not a disk. -//! -//! [`BringUp`] has [`crate::enumerate`]'s shape and for its reason: it is -//! driven to its end in place today, and a driver that gives a scheduler pass -//! back between acts drives the same order. -/// Nanoseconds since boot. -pub type Nanos = u64; +use crate::Nanos; /// The block size everything above this driver is written in; a device whose /// sectors do not divide it is refused, not approximated. @@ -118,7 +113,7 @@ impl core::fmt::Display for Sense { /// One SCSI command, after the transport's own recovery. #[derive(Clone, Copy, PartialEq, Eq, Debug)] -pub enum Outcome { +pub enum Reply { Ok { delivered: u32 }, /// Understood and declined: an optional command's caller must tell "I will /// not" from "I cannot". @@ -138,19 +133,32 @@ pub enum Fail { Budget, } -/// What a disk is, from its READ CAPACITY. +/// What a disk is, from its READ CAPACITY. Only the bring-up sizes one, so +/// every sector it addresses fits READ(10). #[derive(Clone, Copy, PartialEq, Eq, Debug)] pub struct Geometry { - pub sector_bytes: u32, - pub sectors: u64, - pub sectors_per_block: u32, - /// Whole [`HOST_BLOCK`]s. - pub blocks: u64, + sector_bytes: u32, + sectors: u64, + sectors_per_block: u32, + blocks: u64, } impl Geometry { /// A disk not yet asked its size, which no transfer fits. pub const NONE: Self = Self { sector_bytes: 0, sectors: 0, sectors_per_block: 0, blocks: 0 }; + + pub fn sector_bytes(&self) -> u32 { + self.sector_bytes + } + + pub fn sectors(&self) -> u64 { + self.sectors + } + + /// Whole [`HOST_BLOCK`]s. + pub fn blocks(&self) -> u64 { + self.blocks + } } /// A caller's transfer that runs past the disk. @@ -237,20 +245,20 @@ impl Transfer { }) } - /// What `outcome` of `batch` comes to. **Only the first batch may answer + /// What `reply` to `batch` comes to. **Only the first batch may answer /// "ask again"**: blocks already moved are on the device with no way to /// resume. - pub fn answered(&mut self, batch: &Batch, outcome: Outcome) -> Moved { + pub fn answered(&mut self, batch: &Batch, reply: Reply) -> Moved { let first = self.done == 0; - match outcome { - Outcome::Ok { delivered } if delivered as usize == batch.bytes => { + match reply { + Reply::Ok { delivered } if delivered as usize == batch.bytes => { self.done += batch.blocks; Moved::Whole } - Outcome::Ok { delivered } => Moved::Short { delivered }, - Outcome::Refused(sense) => Moved::Refused(sense), - Outcome::Budget if first => Moved::Ended(Fail::Budget), - Outcome::Broken | Outcome::Budget => Moved::Ended(Fail::Device), + Reply::Ok { delivered } => Moved::Short { delivered }, + Reply::Refused(sense) => Moved::Refused(sense), + Reply::Budget if first => Moved::Ended(Fail::Budget), + Reply::Broken | Reply::Budget => Moved::Ended(Fail::Device), } } } @@ -267,13 +275,13 @@ pub enum Flushed { Ended(Fail), } -pub fn flushed(outcome: Outcome) -> Flushed { - match outcome { - Outcome::Refused(sense) if sense.unimplemented() => Flushed::NoCache, - Outcome::Ok { .. } => Flushed::Emptied, - Outcome::Refused(sense) => Flushed::Refused(sense), - Outcome::Broken => Flushed::Ended(Fail::Device), - Outcome::Budget => Flushed::Ended(Fail::Budget), +pub fn flushed(reply: Reply) -> Flushed { + match reply { + Reply::Refused(sense) if sense.unimplemented() => Flushed::NoCache, + Reply::Ok { .. } => Flushed::Emptied, + Reply::Refused(sense) => Flushed::Refused(sense), + Reply::Broken => Flushed::Ended(Fail::Device), + Reply::Budget => Flushed::Ended(Fail::Budget), } } @@ -384,7 +392,6 @@ pub enum Refusal { Unanswered(Query), /// INQUIRY's peripheral device type, not 0 (direct access). NotADisk(u8), - /// A sector size that does not divide [`HOST_BLOCK`]; zero among them. SectorSize(u32), /// A last LBA past what READ(10)'s 32 bits address: serving the first /// 2 TiB of a bigger disk would silently truncate it. @@ -494,6 +501,8 @@ impl BringUp { /// The disk a READ CAPACITY describes, or why it is not one this driver serves. fn geometry(last_lba: u64, sector_bytes: u32) -> Up { + // This driver's set and not SBC-3's, which allows any length: 256 divides + // the host block and is refused. if !matches!(sector_bytes, 512 | 1024 | 2048 | 4096) { return Up::Refused(Refusal::SectorSize(sector_bytes)); } @@ -588,6 +597,9 @@ mod tests { assert!(past(19, 1).is_ok()); let empty = past(u64::MAX, 0).expect("nothing to move"); assert_eq!(empty.next(), None); + let no_size = Transfer::new(0, 1, false, &Geometry::NONE, 8); + assert_eq!(no_size, Err(PastTheEnd { lba: 0, count: 1, blocks: 0 })); + assert_eq!(Transfer::new(0, 0, false, &Geometry::NONE, 8).expect("nothing to move").next(), None); } /// Batches of at most `most` blocks, each at its own sector and offset, @@ -600,9 +612,9 @@ mod tests { let Some(batch) = transfer.next() else { break }; *slot = Some((batch.cdb, batch.blocks, batch.offset, batch.block)); let bytes = batch.bytes as u32; - assert_eq!(transfer.answered(&batch, Outcome::Ok { delivered: bytes - 1 }), Moved::Short { delivered: bytes - 1 }); + assert_eq!(transfer.answered(&batch, Reply::Ok { delivered: bytes - 1 }), Moved::Short { delivered: bytes - 1 }); assert_eq!(transfer.next(), Some(batch), "a short batch is not moved past"); - assert_eq!(transfer.answered(&batch, Outcome::Ok { delivered: bytes }), Moved::Whole); + assert_eq!(transfer.answered(&batch, Reply::Ok { delivered: bytes }), Moved::Whole); } assert_eq!(seen, [ Some((Cdb::transfer(true, 16, 64), 8, 0, 2)), @@ -618,13 +630,13 @@ mod tests { fn only_the_first_batch_may_answer_ask_again() { let mut transfer = Transfer::new(0, 16, false, &STICK, 8).expect("fits"); let first = transfer.next().expect("a batch"); - assert_eq!(transfer.answered(&first, Outcome::Budget), Moved::Ended(Fail::Budget)); - assert_eq!(transfer.answered(&first, Outcome::Broken), Moved::Ended(Fail::Device)); + assert_eq!(transfer.answered(&first, Reply::Budget), Moved::Ended(Fail::Budget)); + assert_eq!(transfer.answered(&first, Reply::Broken), Moved::Ended(Fail::Device)); let sense = Sense { key: 3, asc: 0x11, ascq: 0 }; - assert_eq!(transfer.answered(&first, Outcome::Refused(sense)), Moved::Refused(sense)); - assert_eq!(transfer.answered(&first, Outcome::Ok { delivered: 8 * 4096 }), Moved::Whole); + assert_eq!(transfer.answered(&first, Reply::Refused(sense)), Moved::Refused(sense)); + assert_eq!(transfer.answered(&first, Reply::Ok { delivered: 8 * 4096 }), Moved::Whole); let second = transfer.next().expect("a batch"); - assert_eq!(transfer.answered(&second, Outcome::Budget), Moved::Ended(Fail::Device)); + assert_eq!(transfer.answered(&second, Reply::Budget), Moved::Ended(Fail::Device)); assert!(Moved::Whole.reported() && Moved::Short { delivered: 1 }.reported()); assert!(!Moved::Refused(sense).reported() && !Moved::Ended(Fail::Budget).reported()); } @@ -633,11 +645,11 @@ mod tests { fn a_flush_the_device_does_not_implement_is_no_failure_and_every_other_refusal_is() { let unimplemented = Sense { key: 0x05, asc: 0x20, ascq: 0 }; let failed = Sense { key: 0x04, asc: 0x44, ascq: 0 }; - assert_eq!(flushed(Outcome::Refused(unimplemented)), Flushed::NoCache); - assert_eq!(flushed(Outcome::Refused(failed)), Flushed::Refused(failed)); - assert_eq!(flushed(Outcome::Ok { delivered: 0 }), Flushed::Emptied); - assert_eq!(flushed(Outcome::Broken), Flushed::Ended(Fail::Device)); - assert_eq!(flushed(Outcome::Budget), Flushed::Ended(Fail::Budget)); + assert_eq!(flushed(Reply::Refused(unimplemented)), Flushed::NoCache); + assert_eq!(flushed(Reply::Refused(failed)), Flushed::Refused(failed)); + assert_eq!(flushed(Reply::Ok { delivered: 0 }), Flushed::Emptied); + assert_eq!(flushed(Reply::Broken), Flushed::Ended(Fail::Device)); + assert_eq!(flushed(Reply::Budget), Flushed::Ended(Fail::Budget)); } fn inquiry(peripheral: u8) -> [u8; 36] { @@ -818,10 +830,14 @@ mod tests { assert_eq!(bring_up(disk(&cdrom, &fine), MS).up, Up::Refused(Refusal::NotADisk(0x05))); // The qualifier bits are not the type. assert!(matches!(bring_up(disk(&inquiry(0x20), &fine), MS).up, Up::Ready(_))); - for bytes in [0, 520, 8192, 256] { + for bytes in [0, 520, 8192] { let odd = capacity10(8191, bytes); assert_eq!(bring_up(disk(&disk_inquiry, &odd), MS).up, Up::Refused(Refusal::SectorSize(bytes))); } + // No oracle but the driver's own set: SBC-3 allows 256, which divides + // the host block. + let small = capacity10(8191, 256); + assert_eq!(bring_up(disk(&disk_inquiry, &small), MS).up, Up::Refused(Refusal::SectorSize(256))); let tiny = capacity10(6, 512); assert_eq!( bring_up(disk(&disk_inquiry, &tiny), MS).up, From 95ebbafddf21c3c52e37485328ab9625cffe3868 Mon Sep 17 00:00:00 2001 From: japabu Date: Tue, 29 Sep 2026 21:57:03 +0200 Subject: [PATCH 3/8] The kernel imports CC_SUCCESS and CC_SHORT_PACKET from toyos_xhci::job Both were declared in the driver's mod.rs as well as in the crate, so the kernel's copy answered the crate's machine in msc.rs. The crate's job.rs is the one declaration, as it already is for CC_STALL, and every consumer imports from it. The driver's comment on the pair goes with the constants: the crate's doc on them already says that Short Packet is a success with a residue. Co-Authored-By: Claude Opus 5.5 --- kernel/src/drivers/xhci/device.rs | 4 ++-- kernel/src/drivers/xhci/mod.rs | 5 +---- kernel/src/drivers/xhci/wait/mod.rs | 3 +-- kernel/src/drivers/xhci/wait/msc.rs | 3 ++- 4 files changed, 6 insertions(+), 9 deletions(-) diff --git a/kernel/src/drivers/xhci/device.rs b/kernel/src/drivers/xhci/device.rs index 5e3f52798b..ace0d9cc60 100644 --- a/kernel/src/drivers/xhci/device.rs +++ b/kernel/src/drivers/xhci/device.rs @@ -4,7 +4,7 @@ use crate::log; use toyos_xhci::enumerate::{ self, ep0_packet_from_descriptor, initial_ep0_packet, Act, Enumeration, Learnt, Next, Request, }; -use toyos_xhci::job::{Await, Outcome, Stages}; +use toyos_xhci::job::{Await, Outcome, Stages, CC_SUCCESS}; use toyos_xhci::port::{self, Reset}; use toyos_xhci::identity::UsbId; use toyos_xhci::recovery; @@ -14,7 +14,7 @@ use super::{deadline, Answer, Trb, TrbRing, What, XhciController, PAGE}; use super::{OFF_INPUT_CTX, OFF_DATA_BUF}; use super::{DEV_INT_RING, DEV_EP0_RING, DEV_OUT_CTX, DEV_REPORT, EP0_DCI}; use super::{TRB_ENABLE_SLOT, TRB_ADDRESS_DEVICE, TRB_CONFIGURE_EP, TRB_EVALUATE_CONTEXT}; -use super::{enqueue_control, CC_SUCCESS}; +use super::enqueue_control; use super::hid::{HidType, HidRole, HidDevice}; use super::msc::{Bind, MscInterface, MscRings}; diff --git a/kernel/src/drivers/xhci/mod.rs b/kernel/src/drivers/xhci/mod.rs index 9f6c624a49..52b4e1ce29 100644 --- a/kernel/src/drivers/xhci/mod.rs +++ b/kernel/src/drivers/xhci/mod.rs @@ -22,7 +22,7 @@ use crate::log; use super::pci::PciDevice; use crate::sync::Lock; use toyos_untrusted::Untrusted; -use toyos_xhci::job::{Await, Outcome, Outstanding, Stages}; +use toyos_xhci::job::{Await, Outcome, Outstanding, Stages, CC_SHORT_PACKET, CC_SUCCESS}; use toyos_xhci::port::{self as portmachine, GaveUp, Gone, PortState, Reset, Step}; use toyos_xhci::call::AfterBreak; use toyos_xhci::recovery::{self, Act, EndpointState, NeedsConfigure, Recovery}; @@ -111,9 +111,6 @@ const EVENT_TRANSFER: u32 = 32; const EVENT_CMD_COMPLETE: u32 = 33; const EVENT_PORT_STATUS_CHANGE: u32 = 34; -// CC_SHORT_PACKET is success with a residue, not an error — treating it as one is the classic mass-storage bug. -const CC_SUCCESS: u32 = 1; -const CC_SHORT_PACKET: u32 = 13; const CC_CONTEXT_STATE_ERROR: u32 = 19; /// Stopped, Stopped - Length Invalid and Stopped - Short Packet (Table 6-90): the transfer events a Stop Endpoint raises for the TRB it stopped inside (§4.6.9). const CC_STOPPED: core::ops::RangeInclusive = 26..=28; diff --git a/kernel/src/drivers/xhci/wait/mod.rs b/kernel/src/drivers/xhci/wait/mod.rs index 97d9ce6d7f..ed38c0cb29 100644 --- a/kernel/src/drivers/xhci/wait/mod.rs +++ b/kernel/src/drivers/xhci/wait/mod.rs @@ -39,9 +39,8 @@ mod depth_probe { use crate::log; use super::{deadline, enqueue_control, log_unrecoverable, Completion, Trb, TrbRing}; use super::{XhciController, EVENT_TRANSFER, EVENT_CMD_COMPLETE, USB_TIMEOUT_NS}; -use super::{CC_SUCCESS, CC_SHORT_PACKET}; use toyos_xhci::call::NotTaken; -use toyos_xhci::job::{Await, CC_STALL}; +use toyos_xhci::job::{Await, CC_SHORT_PACKET, CC_STALL, CC_SUCCESS}; use toyos_xhci::recovery::{Act, NeedsConfigure, Recovery}; use toyos_xhci::scan; diff --git a/kernel/src/drivers/xhci/wait/msc.rs b/kernel/src/drivers/xhci/wait/msc.rs index e302d01158..c6f1346422 100644 --- a/kernel/src/drivers/xhci/wait/msc.rs +++ b/kernel/src/drivers/xhci/wait/msc.rs @@ -15,7 +15,7 @@ use super::{Control, Quiet, Restart}; use super::super::{log_unrecoverable, Completion, Disk, StorageGeometry, Trb}; use super::super::{look_for, ports_wanted, with_disk_by, Whereabouts}; use super::super::{TrbRing, XhciController, PAGE, TRB_ADDRESS_DEVICE, TRB_CONFIGURE_EP, TRB_RESET_DEVICE}; -use super::super::{stop, CC_SUCCESS, TRB_NORMAL, OFF_INPUT_CTX}; +use super::super::{stop, TRB_NORMAL, OFF_INPUT_CTX}; use super::super::{AFTER_BREAK, CC_CONTEXT_STATE_ERROR, EP0_DCI}; use super::super::{MSC_IN_RING, MSC_OUT_RING, MSC_CBW, MSC_CSW, MSC_SCRATCH, MSC_SCRATCH_LEN}; use super::super::MSC_INPUT_CTX; @@ -26,6 +26,7 @@ use toyos_xhci::call::{AfterBreak, NotIssued}; use toyos_xhci::configure::{self, BulkEndpoint}; use toyos_xhci::flush::Debt; use toyos_xhci::identity::{self, Identity, Serial, UsbId}; +use toyos_xhci::job::CC_SUCCESS; use toyos_xhci::ladder::{self, AfterReset, Left, PortStep, Run, Rung}; use toyos_xhci::port; use toyos_xhci::reset_recovery::{self, Answered, GaveUp, Look, Pipe, Quiescing, SlotGoes, Step}; From 13ecd3969fd485c2944840d97e8ba14d95493f96 Mon Sep 17 00:00:00 2001 From: japabu Date: Wed, 30 Sep 2026 13:36:48 +0200 Subject: [PATCH 4/8] A rung whose device left its port ends as the device leaving, not as a break MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The T14 reading of `usb_transport_break` at 95ebbafdd: the boot stick sat on USB2 port 1 at high speed. The staged WRITE(10) break left it owed Data-Out, so the ladder entered at the port rung with no class reset (`ladder::enters_at`). The rung's hot reset completed with the port Enabled (PORTSC 0x00200e03); 50 ms later Address Device answered USB Transaction Error. The rung ended Failed, the climb counted break 2 and entered the Offline rung, which read port 1 Disconnected (0x000202a0), sent nothing and said "is offline". The disk was held, the stick bound on port 13 (a USB 3.1 port, speed 4) as the same device, and the WRITE completed on it. Why the rung could not see it leave: a USB2 port detects no disconnect while it drives a reset (xHCI 1.2 §4.19.1.1.2, note 57) and advances to Enabled when the reset ends (§4.19.1.1.4), and a peripheral uses one connection at a time (§4.19.7). A SuperSpeed stick that trains on the USB3 half therefore leaves the USB2 port after the reset's completion has been read, and the rung's next step meets an empty port. - toyos-xhci: `ladder::holds`, connected with no unconsumed connect change, the predicate `reset_port` already applied before writing a reset. - The climb asks it again when a rung ends unverified. A device its port no longer holds ends the climb as one whose reset read the port empty always did: no break counted, no Offline rung, the device left for the hold a reset of this driver's earns it, and its slot the port's teardown's (§4.4). The Offline rung sent nothing to a port that no longer held its device, so nothing changes on the wire; a class rung whose device left no longer runs the port rung's quiesce. - `usb-reset-moves-after` stages that leave under QEMU: the hold comes after the reset's completion is read with the device on the port. QEMU 11.1.1 answers Address Device on a port with no device with TRB Error (hw/usb/hcd-xhci.c, `xhci_address_slot` and `xhci_lookup_uport`), a failed step as the T14's Transaction Error is. `usb_transport_break` gains `Moved::AfterItsReset`, and no same-stick shape may say the stick is offline. - The metal verdict's kernel half is `usb::transport_break_recovered`: the ladder entered at the port rung, the stick is never offline, and either the port rung took and the WRITE completed, or the stick was held, came back as the same device and the WRITE completed on it, each in order. The "Reset Recovery took" alternative is gone, since this break never enters at the class reset. `metal_usb_judge` holds it to the records the T14 wrote at 95ebbafdd (refused) and to the shape this change writes (passed), beside four more. - The SuperSpeed-stick issue gains the reading. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016t9wjdQkB8SH7bmfUoiy6L --- ...ck-enumerates-at-high-speed-under-toyos.md | 7 ++ kernel/src/actuator.rs | 10 +- kernel/src/drivers/xhci/wait/msc.rs | 76 +++++++++---- tests/checks.rs | 7 ++ tests/checks/usb.rs | 91 ++++++++++++++++ tests/common/usb.rs | 100 ++++++++++++------ toyos-xhci/src/ladder.rs | 28 +++++ 7 files changed, 262 insertions(+), 57 deletions(-) create mode 100644 tests/checks/usb.rs diff --git a/issues/hardware/the-t14s-superspeed-stick-enumerates-at-high-speed-under-toyos.md b/issues/hardware/the-t14s-superspeed-stick-enumerates-at-high-speed-under-toyos.md index e28b3e098d..7b123258d5 100644 --- a/issues/hardware/the-t14s-superspeed-stick-enumerates-at-high-speed-under-toyos.md +++ b/issues/hardware/the-t14s-superspeed-stick-enumerates-at-high-speed-under-toyos.md @@ -18,6 +18,13 @@ USB2 half of that receptacle, at speed 3 (high speed): flash. - T14 run 75: port 13, slot 5 — the one boot that began with no power cycle of the stick since run 74's reset had moved it there. +- T14, the `usb_transport_break` boot of PR #588 at 95ebbafdd (2026-09-29): + slot 1, port 1, speed 3, and port 13 did not read connected at the scan. The + scan's own reset of port 1 (0.375 s to 0.430 s) left the stick there; the + port rung's (1.174 s to 1.229 s) did not. That reset's completion read it + Enabled at high speed (`PORTSC 0x00200e03`), port 1 read Disconnected at + 1.279 s (`0x000202a0`), and port 13 connected, link already trained, speed 4, + at 2.210 s. So which half the stick is on at ToyOS's first look is decided before the driver acts, by something between the firmware's hand-off and the boot scan, diff --git a/kernel/src/actuator.rs b/kernel/src/actuator.rs index 7e65a02bbf..efc0cd5e83 100644 --- a/kernel/src/actuator.rs +++ b/kernel/src/actuator.rs @@ -157,11 +157,15 @@ actuators! { usb_reset_break = "usb-reset-break"; /// Hold the port rung's first reset, once, until the port reads empty, so - /// the host can move the device to another port as a reset moved T14 run - /// 79's stick. See `xhci::msc::reset_moves`; judged by - /// `usb_transport_break`. + /// the host can move the device to another port. See + /// `xhci::msc::reset_moves`; judged by `usb_transport_break`. usb_reset_moves = "usb-reset-moves"; + /// The same hold, once the reset's completion has been read with the + /// device on the port: a device that leaves under a USB2 port's reset. + /// See `xhci::msc::reset_moves`; judged by `usb_transport_break`. + usb_reset_moves_after = "usb-reset-moves-after"; + /// `usb-transport-break`'s break, on the first WRITE(10) that goes out /// while its device holds a write it reported complete and no flush has /// emptied: a device that leaves then may have lost it. Judged by diff --git a/kernel/src/drivers/xhci/wait/msc.rs b/kernel/src/drivers/xhci/wait/msc.rs index c6f1346422..9f29a8bbc0 100644 --- a/kernel/src/drivers/xhci/wait/msc.rs +++ b/kernel/src/drivers/xhci/wait/msc.rs @@ -276,6 +276,24 @@ impl core::fmt::Display for Told<'_> { } } +/// A rung that did not bring its device back in step, as its line says it: +/// its TEST UNIT READY broke this way, or a step before it was not answered. +struct Unverified<'a> { + rung: Rung, + broke: Option<&'a Broke>, +} + +impl core::fmt::Display for Unverified<'_> { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + match self.broke { + Some(why) => { + write!(f, "transport broke on the {}'s TEST UNIT READY: {}", self.rung.named(), Told(why)) + } + None => write!(f, "the {} was not answered", self.rung.named()), + } + } +} + /// How one rung of the ladder ended. enum Climbed { /// The device answered the rung's TEST UNIT READY under that command's own @@ -525,8 +543,11 @@ const RECOVERING: &str = "recovering"; /// Hold the port rung's first reset, once, until the port reads empty: QEMU /// cannot move a device off its port on a reset, so the host takes it off and -/// plugs the same backing in on another port, which is T14 run 79's stick -/// leaving the USB2 half for the USB3 one. +/// plugs the same backing in on another port. `usb-reset-moves` holds before +/// the reset's completion is read, so the rung reads the port empty; +/// `usb-reset-moves-after` holds once it has been read with the device on the +/// port, as a USB2 port reads a device that leaves under its reset +/// (`toyos_xhci::ladder::holds`). #[cfg(feature = "boot-actuators")] pub(in crate::drivers::xhci) mod reset_moves { use core::sync::atomic::{AtomicBool, Ordering}; @@ -536,6 +557,10 @@ pub(in crate::drivers::xhci) mod reset_moves { /// What the held reset says, which the host acts on. pub const HELD: &str = "is held empty for the host to move its device (usb-reset-moves)"; + /// What the reset held after its completion was read says. + pub const HELD_AFTER: &str = + "is held, reset with its device on it, for the host to move the device (usb-reset-moves-after)"; + /// The cue the host moves the device on, written to the console directly: /// the record above reaches it only when `klogd` runs, which it may not /// while this CPU spins in the rung, and a cue that arrives after the @@ -546,6 +571,10 @@ pub(in crate::drivers::xhci) mod reset_moves { crate::actuator::usb_reset_moves() && UNSPENT.swap(false, Ordering::Relaxed) } + pub fn take_after() -> bool { + crate::actuator::usb_reset_moves_after() && UNSPENT.swap(false, Ordering::Relaxed) + } + pub fn cue() { crate::drivers::serial::BackendGuard::lock().write_raw(MOVE_NOW); } @@ -1089,17 +1118,11 @@ impl XhciController { return false; } }; - match climbed { + let out_of_step = match climbed { Climbed::InStep => { self.after_break.took(rung); return true; } - Climbed::OutOfStep(why) => { - log!("usb-storage: {slot} transport broke on the {}'s TEST UNIT READY: {}; \ - break {} of {MAX_TRANSPORT_BREAKS} running", - rung.named(), Told(&why), dev.run.breaks().saturating_add(1)); - broke = why.event(); - } // As a round trip whose port read disconnected mid-wait: a // reset aimed at an empty port would only spend its bound. Climbed::Gone => { @@ -1107,14 +1130,24 @@ impl XhciController { dev.left = true; return false; } - Climbed::Failed => { - log!("usb-storage: {slot} the {} was not answered; break {} of \ - {MAX_TRANSPORT_BREAKS} running", rung.named(), dev.run.breaks().saturating_add(1)); - // The event is spent: the rung has commanded the pair - // since, and only the fields speak for it now. - broke = None; - } + Climbed::OutOfStep(why) => Some(why), + Climbed::Failed => None, + }; + let ended = Unverified { rung, broke: out_of_step.as_ref() }; + let port = self.read_portsc(dev.port_idx); + if !ladder::holds(port) { + log!("usb-storage: {slot} {ended}, and port {} no longer holds the device (PORTSC \ + {:#010x}): its port's teardown takes it from here", + u32::from(dev.port_idx) + 1, port.raw()); + dev.failed = true; + dev.left = true; + return false; } + log!("usb-storage: {slot} {ended}; break {} of {MAX_TRANSPORT_BREAKS} running", + dev.run.breaks().saturating_add(1)); + // After a step that was not answered the event is spent: the rung + // has commanded the pair since, and only the fields speak for it now. + broke = out_of_step.as_ref().and_then(Broke::event); // A rung's own TEST UNIT READY has no data phase to be left in. left = Left::Elsewhere; } @@ -1194,9 +1227,7 @@ impl XhciController { let before = self.read_portsc(port_idx); let protocol = self.protocols.of(port_idx); let kind = port::offline_reset(protocol); - // A port that reads empty, or connected across a gap, no longer holds - // the device this is about: its teardown owns what is left. - let here = before.connected() && !before.connect_changed(); + let here = ladder::holds(before); let finished = here && { self.write_portsc(port_idx, port::reset_write(kind, before)); dev.reset_at = Some(crate::clock::nanos_since_boot()); @@ -1239,6 +1270,13 @@ impl XhciController { after.link_state(), after.speed(), ); + #[cfg(feature = "boot-actuators")] + if left == AfterReset::Enumerate && why == RECOVERING && reset_moves::take_after() { + log!("xHCI: {} port {} {}", self.slot(dev.slot_id), u32::from(port_idx) + 1, + reset_moves::HELD_AFTER); + reset_moves::cue(); + let _ = self.settles_within_call(|| !self.read_portsc(port_idx).connected()); + } left } diff --git a/tests/checks.rs b/tests/checks.rs index 183e3c17eb..df5efa5bae 100644 --- a/tests/checks.rs +++ b/tests/checks.rs @@ -17,6 +17,8 @@ mod checks { mod screen_checks; #[path = "serial.rs"] mod serial_checks; + #[path = "usb.rs"] + mod usb_checks; /// One subject: what a console line says died, what a wait does about it, /// and that only one place in the harness answers either. @@ -625,6 +627,11 @@ mod checks { audio_checks::judges_verdict() } + #[test] + fn metal_usb_judge() -> Result<(), String> { + usb_checks::transport_break_verdict() + } + /// `blackbox_unclaimed_page` is a registration `tests/metal-profile.toml` already prices, /// so sizing and batching run for real. #[test] diff --git a/tests/checks/usb.rs b/tests/checks/usb.rs new file mode 100644 index 0000000000..18f64dce40 --- /dev/null +++ b/tests/checks/usb.rs @@ -0,0 +1,91 @@ +use super::*; +use serial::Serial; + +/// The metal verdict on the boot stick's staged transport break, off the +/// records the T14 wrote when its stick left the USB2 half of its receptacle +/// under the port rung's reset: as the ladder told it when it climbed on past +/// the stick that had left, which the verdict has to refuse, and as it tells +/// it now, which it has to pass — beside the shapes on either side of it. +pub fn transport_break_verdict() -> Result<(), String> { + let judged = |what: &str, log: &str, green: bool| { + match (usb::transport_break_recovered(&Serial::named(what, log)), green) { + (Ok(()), true) | (Err(_), false) => Ok(()), + (Ok(()), false) => Err(format!("{what} passed a log it has to refuse:\n{log}")), + (Err(why), true) => Err(format!("{what} refused a log it has to pass: {why}\n{log}")), + } + }; + const STAGED: &str = "\ + [kernel 1.174 cpu3] usb-storage: 00:14.0 slot 1 transport broke on SCSI 0x2a: a staged \ + break skipped the data phase wait; break 1 of 3 running\n"; + const OWED: &str = "\ + [kernel 1.174 cpu3] usb-storage: 00:14.0 slot 1 is owed the data of the command that \ + broke, so nothing can be asked of it on the Bulk-Out: its port is reset with no class \ + reset before it\n"; + const RESET: &str = "\ + [kernel 1.229 cpu3] xHCI: 00:14.0 slot 1 port 1 reset while recovering (hot on a USB2 \ + port): PORTSC 0x00000e03 then 0x00200e03, link Active, speed 3: reset, and the port is \ + enabled\n"; + const UNANSWERED: &str = "\ + [kernel 1.279 cpu3] xHCI: Address Device (after the port reset) failed: code 4 (USB \ + Transaction Error)\n"; + const CLIMBED_ON: &str = "\ + [kernel 1.279 cpu3] usb-storage: 00:14.0 slot 1 the port reset was not answered; break 2 \ + of 3 running\n\ + [kernel 1.279 cpu3] usb-storage: 00:14.0 slot 1 broke 2 times running; its port reset did \ + not bring the transport back\n\ + [kernel 1.279 cpu3] xHCI: 00:14.0 slot 1 port 1 reset while taking it offline (hot on a \ + USB2 port): PORTSC 0x000202a0 then 0x000202a0, link RxDetect, speed 0: nothing is \ + connected, so its port's teardown takes it from here\n\ + [kernel 1.279 cpu3] usb-storage: 00:14.0 slot 1 is offline: both bulk endpoints \ + Stopped=true, port 1 reset=false and nothing sent after it, Reset Device=false, its slot \ + goes back to the controller; every operation on it is refused from here\n"; + const LEFT: &str = "\ + [kernel 1.279 cpu3] usb-storage: 00:14.0 slot 1 the port reset was not answered, and port \ + 1 no longer holds the device (PORTSC 0x000202a0): its port's teardown takes it from here\n"; + const BACK: &str = "\ + [kernel 1.279 cpu3] usb-storage: disk 0 left port 1 (its port read empty) after this \ + driver reset it; it is held 1894 ms for the same device to come back\n\ + [kernel 2.211 cpu0] usb-storage: disk 0 came back on port 13 slot 6 as the same device \ + (USB 0781:5581, serial number \"4C530001310614121352\", 7507812 blocks of 512 B), \ + msc_block +0x30000; its volume carries on\n\ + [kernel 2.259 cpu3] usb-storage: disk 0 is back, and the operation it was asked went out \ + again on it: it completed\n"; + const TOOK: &str = "\ + [kernel 1.330 cpu3] usb-storage: 00:14.0 slot 1 the port reset took: addressed and \ + configured again, the device answered TEST UNIT READY under its own tag 0x5a2\n"; + const COMPLETED: &str = "\ + [kernel 1.331 cpu3] usb-storage: 00:14.0 slot 1 SCSI 0x2a completed after 1 break(s) \ + running; the transport came back and the count is cleared\n"; + const CLASS_RESET_TOOK: &str = "\ + [kernel 1.175 cpu3] usb-storage: 00:14.0 slot 1 Reset Recovery took: the device answered \ + TEST UNIT READY under its own tag 0x5a2\n"; + const CLASS_RESET_UNANSWERED: &str = "\ + [kernel 1.175 cpu3] usb-storage: 00:14.0 slot 1 the class reset was not answered; break 2 \ + of 3 running\n"; + + judged("the break as the T14 read it", &format!("{STAGED}{OWED}{RESET}{UNANSWERED}{CLIMBED_ON}{BACK}"), false)?; + judged("the break its stick left", &format!("{STAGED}{OWED}{RESET}{UNANSWERED}{LEFT}{BACK}"), true)?; + let lost = BACK.replace( + "came back on port 13 slot 6 as the same device", + "did not come back within 2000 ms of its port reset; it is offline", + ); + judged("a stick that never came back", &format!("{STAGED}{OWED}{RESET}{UNANSWERED}{LEFT}{lost}"), false)?; + let failed = BACK.replace("it completed", "it failed"); + judged( + "a write that failed on the stick that came back", + &format!("{STAGED}{OWED}{RESET}{UNANSWERED}{LEFT}{failed}"), + false, + )?; + judged("a stick that answered on its port", &format!("{STAGED}{OWED}{RESET}{TOOK}{COMPLETED}"), true)?; + judged( + "a stick owed a write's data that the class reset brought back", + &format!("{STAGED}{CLASS_RESET_TOOK}{COMPLETED}"), + false, + )?; + judged( + "a stick owed a write's data given the class reset before its port reset moved it", + &format!("{STAGED}{CLASS_RESET_UNANSWERED}{RESET}{UNANSWERED}{LEFT}{BACK}"), + false, + )?; + Ok(()) +} diff --git a/tests/common/usb.rs b/tests/common/usb.rs index dd21fa8dac..f4527480d1 100644 --- a/tests/common/usb.rs +++ b/tests/common/usb.rs @@ -1520,6 +1520,7 @@ pub fn usb_transport_break( transport_gives_up(test_config, c_bins, rust_bins)?; abandoned_write_is_taken_offline(test_config, c_bins, rust_bins)?; a_stick_its_reset_moved_carries_on(Moved::SameStick)?; + a_stick_its_reset_moved_carries_on(Moved::AfterItsReset)?; a_stick_its_reset_moved_carries_on(Moved::AnotherStick)?; a_stick_its_reset_moved_carries_on(Moved::SlowStick)?; a_stick_its_reset_moved_carries_on(Moved::OwedFlush)?; @@ -1631,6 +1632,11 @@ enum Moved { /// reset can move a stick from the USB2 half of its receptacle to the USB3 /// half. SameStick, + /// The stick itself, moved only once its port's reset has been read + /// complete with it on the port (`usb-reset-moves-after`): a device that + /// leaves under a USB2 port's reset. The rung's next step fails on an empty + /// port, and the rung ends as the stick leaving rather than as a break. + AfterItsReset, /// The same backing under another serial number, which is everything a /// second unit of the same model shares with the first — INQUIRY, /// capacity, USB ids — and the negative control: it must not be adopted. @@ -1671,6 +1677,8 @@ enum Moved { /// the bind is another CPU's, read off the kernel's own stamps. fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { const HELD: &str = "is held empty for the host to move its device (usb-reset-moves)"; + const HELD_AFTER: &str = + "is held, reset with its device on it, for the host to move the device (usb-reset-moves-after)"; const MOVE_NOW: &str = "usb-reset-moves: move the device now"; const STALLED: &str = "answers slowly (usb-slow-return): its bind is stalled"; const OWED: &str = ", and it left owing a flush of writes it had reported complete, so the \ @@ -1688,6 +1696,7 @@ fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { out again on it"; let params: &'static [&'static str] = match moved { Moved::SameStick | Moved::AnotherStick => &["usb-transport-break", "usb-reset-moves"], + Moved::AfterItsReset => &["usb-transport-break", "usb-reset-moves-after"], Moved::SlowStick => &["usb-transport-break", "usb-reset-moves", "usb-slow-return"], Moved::OwedFlush => &["usb-transport-break-owed", "usb-reset-moves"], Moved::FlushedStick => &["usb-transport-break-flushed", "usb-reset-moves"], @@ -1696,6 +1705,7 @@ fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { let case = super::compile::repo_root().join("tests/jobcase"); let (name, serial) = match moved { Moved::SameStick => ("usb-reset-moves-same.img", qemu::BOOT_STICK_SERIAL), + Moved::AfterItsReset => ("usb-reset-moves-after.img", qemu::BOOT_STICK_SERIAL), Moved::AnotherStick => ("usb-reset-moves-another.img", "TOYOS0OTHERSTICK"), Moved::SlowStick => ("usb-reset-moves-slow.img", qemu::BOOT_STICK_SERIAL), Moved::OwedFlush => ("usb-reset-moves-owed.img", qemu::BOOT_STICK_SERIAL), @@ -1738,7 +1748,7 @@ fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { // which may come on either side of it. let ends = |c: &str| { let last = match moved { - Moved::SameStick | Moved::SlowStick | Moved::FlushedStick => { + Moved::SameStick | Moved::AfterItsReset | Moved::SlowStick | Moved::FlushedStick => { c.contains(toyos_build::bootlog::REBOOTING) } Moved::AnotherStick => c.contains(" did not come back within "), @@ -1768,16 +1778,27 @@ fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { } Ok(()) }; - let left = [ - format!("usb-storage: {under_test} is owed the data of the command that broke"), - HELD.to_string(), + let held = " after this driver reset it; it is held ".to_string(); + let mut left = vec![format!("usb-storage: {under_test} is owed the data of the command that broke")]; + if moved == Moved::AfterItsReset { + left.extend([ + HELD_AFTER.to_string(), + format!( + "usb-storage: {under_test} the port reset was not answered, and port 1 no longer \ + holds the device (PORTSC " + ), + ]); + } else { + left.push(HELD.to_string()); + } + left.extend([ // Its port read empty inside the rung, or — when the host's move came // after the rung's bound and the reset verified — disconnected at the // next look: run 79's shape and run 74's, both a device that left // under a reset of this driver's. "usb-storage: disk 0 left port 1 (".to_string(), - " after this driver reset it; it is held ".to_string(), - ]; + held.clone(), + ]); let inside_the_rung = log.contains("usb-storage: disk 0 left port 1 (its port read empty)"); let back = [ "xHCI: port 3 connected".to_string(), @@ -1818,7 +1839,7 @@ fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { }; let came_back = "usb-storage: disk 0 came back on port 3 slot "; match moved { - Moved::SameStick | Moved::SlowStick | Moved::FlushedStick => { + Moved::SameStick | Moved::AfterItsReset | Moved::SlowStick | Moved::FlushedStick => { if moved == Moved::FlushedStick { let flushed = line_with(&log, AFTER_A_FLUSH)?; if !log.split_once(staged).is_some_and(|(before, _)| before.contains(flushed)) { @@ -1827,6 +1848,7 @@ fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { } // First, and the debt first of all, so a debt the stick came back // with is named as one. + let offline = format!("usb-storage: {under_test} is offline"); for never in [ OWED, FLUSH_LOST, @@ -1834,6 +1856,7 @@ fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { " did not come back within ", "disk 1 ready", " is not disk 0 come back", + offline.as_str(), ] { if let Some(line) = log.lines().find(|l| l.contains(never)) { return Err(format!("{moved:?}: {line:?} of a stick that came back as itself\n{log}")); @@ -1852,11 +1875,11 @@ fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { // stall, or every CPU inside a call on the held disk, so no CPU // took the pass that binds — and the write was asked again. let shape = if moved != Moved::SlowStick && held_end.ends_with(": it completed") { - in_order(&[left[3].clone(), came_back.to_string(), held_end.to_string()])?; + in_order(&[held.clone(), came_back.to_string(), held_end.to_string()])?; held_call(came_back)?; "the write that waited went out again on it" } else if held_end.contains(STILL_HELD) { - in_order(&[left[3].clone(), held_end.to_string()])?; + in_order(&[held.clone(), held_end.to_string()])?; held_call(if moved == Moved::SlowStick { STALLED } else { came_back })?; "the call that waited ended on its bound and the write was asked again" } else { @@ -1876,6 +1899,7 @@ fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { "after the rung's reset verified" }, match moved { + Moved::AfterItsReset => ", once its reset had been read complete with it there", Moved::SlowStick => ", though its bind was stalled past the window", Moved::FlushedStick => { ", owing no flush: it broke after a flush that succeeded over its last write" @@ -2108,44 +2132,50 @@ fn no_command_was_refused(log: &str) -> Result<(), String> { } /// The staged break on a real stick: the transfer abandoned on the boot stick's -/// first WRITE(10) is recovered, the write completes, the disk stays online, -/// and the boot goes on to the deliberate reboot that ends its chain. -/// -/// Which rung brought it back is the stick's to decide — one that does not -/// honour the class reset is brought back by the port reset — so that one of -/// them verified is asserted, and which is printed. +/// first WRITE(10) is recovered, the write completes, the disk keeps its +/// number, and the boot goes on to the deliberate reboot that ends its chain. pub fn transport_break_on_metal( kernel: &serial::Serial, after: &serial::Serial, ) -> Result<(), String> { + transport_break_recovered(kernel)?; + super::power::done_chain(after) +} + +/// The kernel log's half of [`transport_break_on_metal`]. +/// +/// **The ladder enters at the port reset**: the break leaves the stick owed a +/// WRITE's data, across which no class reset may be asked. The stick decides +/// the rest. It answers the rung's TEST UNIT READY on its port, and the write +/// goes out again there; or it leaves its port under the reset — a SuperSpeed +/// stick enumerated on the USB2 half of its receptacle trains on the USB3 half +/// — is held, comes back as the same device, and the write goes out again on +/// it. **Either way no rung takes it offline.** +pub fn transport_break_recovered(kernel: &serial::Serial) -> Result<(), String> { let staged = kernel.must_say( "transport broke on SCSI 0x2a: a staged break skipped the data phase wait; break 1 of ", )?; let under_test = broke_on(staged)?; - // T14 runs 74 and 79: the port reset can move the stick to the other half - // of its receptacle. Then no rung verifies on the old slot, and what says - // the volume carried on is the same device taking its disk number back and - // the command that broke completing on it. - if let Ok(left) = kernel.must_say(" after this driver reset it; it is held ") { + let entered = kernel.must_say_after( + staged, + &format!("usb-storage: {under_test} is owed the data of the command that broke"), + )?; + kernel.must_not_say(&format!("usb-storage: {under_test} is offline"))?; + kernel.must_not_say(" did not come back within ")?; + if let Ok(left) = kernel.must_say_after(entered, " after this driver reset it; it is held ") { + let back = kernel.must_say_after(left, " as the same device (USB ")?; + kernel.must_say_after( + back, + "is back, and the operation it was asked went out again on it: it completed", + )?; eprintln!(" [usb] {left}"); - let back = kernel.must_say(" as the same device (USB ")?; eprintln!(" [usb] {back}"); - kernel.must_say("is back, and the operation it was asked went out again on it: it completed")?; - kernel.must_not_say(" did not come back within ")?; - return super::power::done_chain(after); + return Ok(()); } - let rungs = [ - format!("usb-storage: {under_test} Reset Recovery took"), - format!("usb-storage: {under_test} the port reset took"), - ]; - let took = rungs - .iter() - .find_map(|rung| kernel.must_say(rung).ok()) - .ok_or_else(|| format!("neither {:?} nor {:?}: no rung verified", rungs[0], rungs[1]))?; + let took = kernel.must_say_after(entered, &format!("usb-storage: {under_test} the port reset took"))?; + kernel.must_say_after(took, &format!("usb-storage: {under_test} SCSI 0x2a completed after "))?; eprintln!(" [usb] {took}"); - kernel.must_say(&format!("usb-storage: {under_test} SCSI 0x2a completed after "))?; - kernel.must_not_say(&format!("usb-storage: {under_test} is offline"))?; - super::power::done_chain(after) + Ok(()) } /// A read whose port reads gone is a break the driver does not recover: no diff --git a/toyos-xhci/src/ladder.rs b/toyos-xhci/src/ladder.rs index 380d59896a..dc9ddc9361 100644 --- a/toyos-xhci/src/ladder.rs +++ b/toyos-xhci/src/ladder.rs @@ -31,6 +31,8 @@ //! again asks the same question of the same device: the next break climbs on. //! Only a completed round trip ends the run. +use crate::portsc::Portsc; + /// One rung of the ladder. #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Debug)] pub enum Rung { @@ -185,6 +187,20 @@ pub const PORT_RESET: [PortStep; 7] = [ /// the 50 ms Linux's hub driver waits, for the firmware that needs it. pub const RESET_RECOVERY_NS: u64 = 50_000_000; +/// Whether a port still holds the device a rung is about: connected, and with +/// no connect change the driver has not consumed, which would be a disconnect +/// (xHCI 1.2 §5.4.8's CSC). +/// +/// **Asked again when a rung ends unverified**, because the reset's completion +/// cannot answer it on a USB2 port: the port detects no disconnect while it +/// drives the reset (§4.19.1.1.2, note 57) and reads Enabled once the reset +/// ends (§4.19.1.1.4), whether or not its device then stays. A device its port +/// no longer holds left: that is no break, no rung above reaches it, and its +/// port's teardown owns what it held (§4.4). +pub fn holds(port: Portsc) -> bool { + port.connected() && !port.connect_changed() +} + /// Whether a failed `step` ends [`Rung::PortReset`]. pub fn ends_the_rung(step: PortStep) -> bool { step != PortStep::Quiesce @@ -360,6 +376,18 @@ mod tests { } } + /// The T14's USB2 port under the port rung's reset: before it, at its + /// completion, and once its SuperSpeed stick had left for the USB3 half of + /// the receptacle. + #[test] + fn a_port_holds_its_device_only_while_it_reads_connected_with_no_change_unconsumed() { + assert!(holds(Portsc::from_raw(0x0000_0e03)), "Enabled at high speed"); + assert!(holds(Portsc::from_raw(0x0020_0e03)), "a reset that ended is no connect change"); + assert!(!holds(Portsc::from_raw(0x0002_02a0)), "Disconnected (§4.19.1.1.2), unconsumed"); + assert!(!holds(Portsc::from_raw(0x0000_02a0)), "Disconnected, its change consumed"); + assert!(!holds(Portsc::from_raw(0x0002_02e1)), "Disabled on a connect since: another connection"); + } + #[test] fn a_device_is_addressed_again_only_as_what_it_was_enumerated_as() { assert_eq!(after_reset(true, true, true, 3, 3), AfterReset::Enumerate); From a2b229f4e832a9bb3d2716962342c7496c7bf66c Mon Sep 17 00:00:00 2001 From: japabu Date: Wed, 30 Sep 2026 13:51:22 +0200 Subject: [PATCH 5/8] The moved-stick judge builds its held needle where it uses it `cargo run -- --ci host` reddened on clippy::redundant_clone at the two `held.clone()` calls 13ecd3969 added to `a_stick_its_reset_moved_carries_on`: each was the value's last use. The needle is a `&str` now, made a `String` at each use. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016t9wjdQkB8SH7bmfUoiy6L --- tests/common/usb.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/common/usb.rs b/tests/common/usb.rs index f4527480d1..1f98e47e78 100644 --- a/tests/common/usb.rs +++ b/tests/common/usb.rs @@ -1778,7 +1778,7 @@ fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { } Ok(()) }; - let held = " after this driver reset it; it is held ".to_string(); + let held = " after this driver reset it; it is held "; let mut left = vec![format!("usb-storage: {under_test} is owed the data of the command that broke")]; if moved == Moved::AfterItsReset { left.extend([ @@ -1797,7 +1797,7 @@ fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { // next look: run 79's shape and run 74's, both a device that left // under a reset of this driver's. "usb-storage: disk 0 left port 1 (".to_string(), - held.clone(), + held.to_string(), ]); let inside_the_rung = log.contains("usb-storage: disk 0 left port 1 (its port read empty)"); let back = [ @@ -1875,11 +1875,11 @@ fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { // stall, or every CPU inside a call on the held disk, so no CPU // took the pass that binds — and the write was asked again. let shape = if moved != Moved::SlowStick && held_end.ends_with(": it completed") { - in_order(&[held.clone(), came_back.to_string(), held_end.to_string()])?; + in_order(&[held.to_string(), came_back.to_string(), held_end.to_string()])?; held_call(came_back)?; "the write that waited went out again on it" } else if held_end.contains(STILL_HELD) { - in_order(&[held.clone(), held_end.to_string()])?; + in_order(&[held.to_string(), held_end.to_string()])?; held_call(if moved == Moved::SlowStick { STALLED } else { came_back })?; "the call that waited ended on its bound and the write was asked again" } else { From 1aa3a1b81e1c4116a6ac31a8ab5f1d6db4f24639 Mon Sep 17 00:00:00 2001 From: japabu Date: Wed, 30 Sep 2026 16:36:59 +0200 Subject: [PATCH 6/8] One exit for a device its port no longer holds, and a staging that breaks the rung's TEST UNIT READY on it Answers the third review of #588 at dc0863b11. - BLOCKER 2, one exit for one fact: `Climbed::Gone` is deleted. The port rung's reset step now fails whenever `reset_port` answers anything but `Enumerate`, `AfterReset::Left` included, and the climb's `!holds` read is the one place a rung ends as its device leaving. Every `AfterReset::Left` reads `!holds` there: a port read empty reads connected again only on a new connect, which raises CSC, and the only acknowledge between the two reads is `enumeration_ack`, which clears CSC only after a warm reset whose `after` word carried it. - BLOCKER 1: `usb-reset-moves-configured` holds the port rung once every step was answered and before its TEST UNIT READY, and cues the host to move the stick. QEMU 11.1.1 detaches the slot's port before it updates PORTSC (hw/usb/hcd-xhci.c, `xhci_detach` then `xhci_detach_slot`), and a doorbell on a slot whose port is detached kicks nothing (`xhci_kick_epctx`, `xhci_slot_ok`), so the CBW's wait reads the port disconnected: the rung ends `OutOfStep(Broke::Gone)` on a port that no longer holds the stick. `usb_transport_break` gains `Moved::BeforeItsTestUnitReady`, which asserts `transport broke on the port reset's TEST UNIT READY: the port disconnected during the command phase, and port 1 no longer holds the device` and, with every same-stick shape, no `is offline`. - `holds` moves onto `Portsc`, and the port machine, the HID recovery, the climb and `reset_port` read it; the two site comments that restated it go. - One helper, `hold_for_the_move`, for the three staging holds, and one `reset_moves::take` that takes the actuator it is asked for. - The metal verdict no longer refuses `did not come back within`: no case needed it, since a stick that never came back is refused by its missing same-device line first. - REMOVEs: the chronology in `transport_break_verdict`'s doc, and ", the T14 has not yet" in the offline-disk issue. - The SuperSpeed-stick issue carries the open question of why the port rung's reset moved the stick where the scan's did not. - Filed: a USB3 device attached but untrained reads CAS with CCS clear, and so reads as gone. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016t9wjdQkB8SH7bmfUoiy6L --- ...ck-enumerates-at-high-speed-under-toyos.md | 8 +- ...isk-taken-offline-is-never-brought-back.md | 2 +- ...ce-attached-but-untrained-reads-as-gone.md | 29 ++++++ kernel/src/actuator.rs | 6 ++ kernel/src/drivers/xhci/mod.rs | 7 +- kernel/src/drivers/xhci/wait/msc.rs | 90 +++++++++---------- tests/checks/usb.rs | 4 +- tests/common/usb.rs | 50 ++++++++--- toyos-xhci/src/ladder.rs | 28 ------ toyos-xhci/src/portsc.rs | 20 +++++ 10 files changed, 149 insertions(+), 95 deletions(-) create mode 100644 issues/kernel/a-usb3-device-attached-but-untrained-reads-as-gone.md diff --git a/issues/hardware/the-t14s-superspeed-stick-enumerates-at-high-speed-under-toyos.md b/issues/hardware/the-t14s-superspeed-stick-enumerates-at-high-speed-under-toyos.md index 7b123258d5..e13b2a4026 100644 --- a/issues/hardware/the-t14s-superspeed-stick-enumerates-at-high-speed-under-toyos.md +++ b/issues/hardware/the-t14s-superspeed-stick-enumerates-at-high-speed-under-toyos.md @@ -35,8 +35,12 @@ lose the volume's disk number (`issues/kernel/a-disk-taken-offline-is-never-brought-back.md`). Not investigated. What is not known: what the firmware leaves the two ports in, -whether the boot scan's own reset of port 1 is what keeps the stick there, and -whether the USB3 port reads connected at the scan. +whether the boot scan's own reset of port 1 is what keeps the stick there, +whether the USB3 port reads connected at the scan, and why the port rung's +reset moved the stick where the scan's reset of the same port 0.8 s earlier +did not. xHCI 1.2 says why the USB2 port cannot see the stick leave, not why +the stick leaves: that is the USB 3.x specification's device-side rule, which +nobody here has read. ## Exit condition diff --git a/issues/kernel/a-disk-taken-offline-is-never-brought-back.md b/issues/kernel/a-disk-taken-offline-is-never-brought-back.md index 066da2ef0d..6b94c361b4 100644 --- a/issues/kernel/a-disk-taken-offline-is-never-brought-back.md +++ b/issues/kernel/a-disk-taken-offline-is-never-brought-back.md @@ -21,7 +21,7 @@ Two ways a disk that could still be served is lost today: training SuperSpeed on the other half, so it left port 1 and arrived on port 13. `toyos_xhci::identity` now holds such a disk for `RETURN_WINDOW` and gives its number back to a device that proves the same identity; QEMU - judges it (`usb_transport_break`'s moved boots), the T14 has not yet. + judges it (`usb_transport_break`'s moved boots). - **Offline is for the connection's life.** Nothing asks an offline device again, however long it stays plugged in. diff --git a/issues/kernel/a-usb3-device-attached-but-untrained-reads-as-gone.md b/issues/kernel/a-usb3-device-attached-but-untrained-reads-as-gone.md new file mode 100644 index 0000000000..d39e2b58aa --- /dev/null +++ b/issues/kernel/a-usb3-device-attached-but-untrained-reads-as-gone.md @@ -0,0 +1,29 @@ +--- +status: open +kind: defect +opened: 2026-09-30 +--- + +# A USB3 device attached but untrained reads as gone + +Derived from the specification and the code, not staged and not seen on the T14. + +A USB3 root-hub port that detected a device it could not bring to Enabled +reports it with CAS set and CCS clear (xHCI 1.2 §5.4.8, PORTSC bit 24), and a +warm reset clears it. `toyos_xhci::portsc::Portsc` names no CAS, and nothing +in `toyos-xhci` or `kernel/src/drivers/xhci` warm-resets a port for it. Such a +port reads `!Portsc::holds()`: + +- the port machine (`toyos_xhci::port::PortState::step`) keys on CCS, so it + tears down what the port held and never enumerates the device; +- the recovery ladder (`climb` and `reset_port` in + `kernel/src/drivers/xhci/wait/msc.rs`) ends a rung whose stick is in that + state as the stick leaving, and resets nothing. + +So a disk whose device lands in that state is lost until it is replugged. +Linux reads CAS as a connection and warm-resets the port +(`xhci_hub_report_usb3_link_state` in `drivers/usb/host/xhci-hub.c`). + +**Exit**: `Portsc` names CAS; the port machine warm-resets a USB3 port that +reads it and enumerates what trains; the ladder does not take such a device +for one that left. diff --git a/kernel/src/actuator.rs b/kernel/src/actuator.rs index efc0cd5e83..a8bd890978 100644 --- a/kernel/src/actuator.rs +++ b/kernel/src/actuator.rs @@ -166,6 +166,12 @@ actuators! { /// See `xhci::msc::reset_moves`; judged by `usb_transport_break`. usb_reset_moves_after = "usb-reset-moves-after"; + /// The same hold, once the port rung has configured the device again and + /// before its TEST UNIT READY: a device that leaves after every step of + /// the rung was answered. See `xhci::msc::reset_moves`; judged by + /// `usb_transport_break`. + usb_reset_moves_configured = "usb-reset-moves-configured"; + /// `usb-transport-break`'s break, on the first WRITE(10) that goes out /// while its device holds a write it reported complete and no flush has /// emptied: a device that leaves then may have lost it. Judged by diff --git a/kernel/src/drivers/xhci/mod.rs b/kernel/src/drivers/xhci/mod.rs index 52b4e1ce29..0cb0f324bb 100644 --- a/kernel/src/drivers/xhci/mod.rs +++ b/kernel/src/drivers/xhci/mod.rs @@ -975,11 +975,9 @@ impl XhciController { (dev.ep_addr, dev.int_ep_dci, dev.port_idx, dev.block); // Disconnect wins the race: a transaction-error code from a pulled device is indistinguishable from a bad cable, only the port register tells them apart. - // - // CSC as well as CCS: a replug reads connected again but the transfer still died with the old device. let slot = self.slot(slot_id); let portsc = self.read_portsc(port_idx); - if !portsc.connected() || portsc.connect_changed() { + if !portsc.holds() { log!("xHCI: USB {kind} on {slot}: interrupt endpoint {ep_addr:#04x} \ completed with {} as its port went away; leaving it to the disconnect", Completion(code)); @@ -1179,8 +1177,7 @@ impl XhciController { const MAX_EFFECTS: usize = 16; for _ in 0..MAX_EFFECTS { let portsc = self.read_portsc(port_idx); - // CCS or CSC: a replug between two looks reads connected again, but the device that was here has still gone. - if !portsc.connected() || portsc.connect_changed() { + if !portsc.holds() { self.cancel_recovery_on(port_idx); device::cancel_on(self, port_idx); } diff --git a/kernel/src/drivers/xhci/wait/msc.rs b/kernel/src/drivers/xhci/wait/msc.rs index 9f29a8bbc0..a083a58799 100644 --- a/kernel/src/drivers/xhci/wait/msc.rs +++ b/kernel/src/drivers/xhci/wait/msc.rs @@ -305,9 +305,6 @@ enum Climbed { /// A step before the TEST UNIT READY was not answered, and said so; the /// device was asked nothing after it. Failed, - /// The port reads empty: the device is no longer on the bus, and its - /// port's teardown owns what it held. - Gone, } /// Abandon one bulk transfer without waiting, once per boot, on the first @@ -541,25 +538,26 @@ pub(in crate::drivers::xhci) mod reset_break { /// What the port rung's reset is made for, in its line. const RECOVERING: &str = "recovering"; -/// Hold the port rung's first reset, once, until the port reads empty: QEMU -/// cannot move a device off its port on a reset, so the host takes it off and -/// plugs the same backing in on another port. `usb-reset-moves` holds before -/// the reset's completion is read, so the rung reads the port empty; -/// `usb-reset-moves-after` holds once it has been read with the device on the -/// port, as a USB2 port reads a device that leaves under its reset -/// (`toyos_xhci::ladder::holds`). +/// Hold the port rung, once, until its port reads empty: QEMU cannot move a +/// device off its port on a reset, so the host takes it off and plugs the same +/// backing in on another port. `usb-reset-moves` holds before the reset's +/// completion is read, so the rung reads the port empty; +/// `usb-reset-moves-after` once it has been read with the device on the port, +/// as a USB2 port reads a device that leaves under its reset; +/// `usb-reset-moves-configured` once the rung has configured the device again, +/// so its TEST UNIT READY meets an empty port. #[cfg(feature = "boot-actuators")] pub(in crate::drivers::xhci) mod reset_moves { use core::sync::atomic::{AtomicBool, Ordering}; static UNSPENT: AtomicBool = AtomicBool::new(true); - /// What the held reset says, which the host acts on. + /// What each hold says, which the host acts on. pub const HELD: &str = "is held empty for the host to move its device (usb-reset-moves)"; - - /// What the reset held after its completion was read says. pub const HELD_AFTER: &str = "is held, reset with its device on it, for the host to move the device (usb-reset-moves-after)"; + pub const HELD_CONFIGURED: &str = "is held, configured again, for the host to move the device \ + (usb-reset-moves-configured)"; /// The cue the host moves the device on, written to the console directly: /// the record above reaches it only when `klogd` runs, which it may not @@ -567,12 +565,9 @@ pub(in crate::drivers::xhci) mod reset_moves { /// rung's bound stages a device that left too late. const MOVE_NOW: &[u8] = b"usb-reset-moves: move the device now\n"; - pub fn take() -> bool { - crate::actuator::usb_reset_moves() && UNSPENT.swap(false, Ordering::Relaxed) - } - - pub fn take_after() -> bool { - crate::actuator::usb_reset_moves_after() && UNSPENT.swap(false, Ordering::Relaxed) + /// Whether the hold `staged` arms is taken here: one hold per boot. + pub fn take(staged: bool) -> bool { + staged && UNSPENT.swap(false, Ordering::Relaxed) } pub fn cue() { @@ -1123,19 +1118,17 @@ impl XhciController { self.after_break.took(rung); return true; } - // As a round trip whose port read disconnected mid-wait: a - // reset aimed at an empty port would only spend its bound. - Climbed::Gone => { - dev.failed = true; - dev.left = true; - return false; - } Climbed::OutOfStep(why) => Some(why), Climbed::Failed => None, }; let ended = Unverified { rung, broke: out_of_step.as_ref() }; + // A device its port no longer holds left, which is no break and + // which no rung above reaches: its port's teardown owns what it + // held. Asked of the port here, since a USB2 port detects no + // disconnect while it drives a reset (xHCI 1.2 §4.19.1.1.2, note + // 57) and reads Enabled once the reset ends (§4.19.1.1.4). let port = self.read_portsc(dev.port_idx); - if !ladder::holds(port) { + if !port.holds() { log!("usb-storage: {slot} {ended}, and port {} no longer holds the device (PORTSC \ {:#010x}): its port's teardown takes it from here", u32::from(dev.port_idx) + 1, port.raw()); @@ -1227,18 +1220,15 @@ impl XhciController { let before = self.read_portsc(port_idx); let protocol = self.protocols.of(port_idx); let kind = port::offline_reset(protocol); - let here = ladder::holds(before); + let here = before.holds(); let finished = here && { self.write_portsc(port_idx, port::reset_write(kind, before)); dev.reset_at = Some(crate::clock::nanos_since_boot()); self.settles_within_call(|| self.read_portsc(port_idx).reset_finished()) }; #[cfg(feature = "boot-actuators")] - if finished && why == RECOVERING && reset_moves::take() { - log!("xHCI: {} port {} {}", self.slot(dev.slot_id), u32::from(port_idx) + 1, - reset_moves::HELD); - reset_moves::cue(); - let _ = self.settles_within_call(|| !self.read_portsc(port_idx).connected()); + if finished && why == RECOVERING && reset_moves::take(crate::actuator::usb_reset_moves()) { + self.hold_for_the_move(dev, reset_moves::HELD); } let after = self.read_portsc(port_idx); if finished { @@ -1271,15 +1261,25 @@ impl XhciController { after.speed(), ); #[cfg(feature = "boot-actuators")] - if left == AfterReset::Enumerate && why == RECOVERING && reset_moves::take_after() { - log!("xHCI: {} port {} {}", self.slot(dev.slot_id), u32::from(port_idx) + 1, - reset_moves::HELD_AFTER); - reset_moves::cue(); - let _ = self.settles_within_call(|| !self.read_portsc(port_idx).connected()); + if left == AfterReset::Enumerate + && why == RECOVERING + && reset_moves::take(crate::actuator::usb_reset_moves_after()) + { + self.hold_for_the_move(dev, reset_moves::HELD_AFTER); } left } + /// Say `held`, cue the host, and hold the rung until the device's port + /// reads empty (`reset_moves`). + #[cfg(feature = "boot-actuators")] + fn hold_for_the_move(&self, dev: &MscDevice, held: &str) { + let port_idx = dev.port_idx; + log!("xHCI: {} port {} {held}", self.slot(dev.slot_id), u32::from(port_idx) + 1); + reset_moves::cue(); + let _ = self.settles_within_call(|| !self.read_portsc(port_idx).connected()); + } + /// The ladder's second rung: the port reset, and the enumeration a reset /// owes (xHCI 1.2 §4.19.5), on the slot, the pool block and the disk number /// the device already has — so a mount on it carries on. The steps and @@ -1293,14 +1293,8 @@ impl XhciController { PortStep::Quiesce => { self.quiesce_bulk_pair(dev, broke, "stopping it before its port is reset") } - PortStep::Reset => match self.reset_port(dev, RECOVERING) { - AfterReset::Enumerate => true, - AfterReset::Left => return Climbed::Gone, - // `reset_port` has said which. - AfterReset::NeverFinished - | AfterReset::NotEnabled - | AfterReset::SpeedChanged { .. } => false, - }, + // `reset_port` has said which way it did not. + PortStep::Reset => self.reset_port(dev, RECOVERING) == AfterReset::Enumerate, PortStep::Settle => { let _ = crate::clock::settles( self.after_break @@ -1339,6 +1333,10 @@ impl XhciController { return Climbed::Failed; } } + #[cfg(feature = "boot-actuators")] + if reset_moves::take(crate::actuator::usb_reset_moves_configured()) { + self.hold_for_the_move(dev, reset_moves::HELD_CONFIGURED); + } match self.bot(dev, &Cdb::TEST_UNIT_READY, None, Asks::Verification(Rung::PortReset)) { Ok(answer) => { log!("usb-storage: {slot} the port reset took: addressed and configured again, the \ diff --git a/tests/checks/usb.rs b/tests/checks/usb.rs index 18f64dce40..901770714d 100644 --- a/tests/checks/usb.rs +++ b/tests/checks/usb.rs @@ -3,9 +3,7 @@ use serial::Serial; /// The metal verdict on the boot stick's staged transport break, off the /// records the T14 wrote when its stick left the USB2 half of its receptacle -/// under the port rung's reset: as the ladder told it when it climbed on past -/// the stick that had left, which the verdict has to refuse, and as it tells -/// it now, which it has to pass — beside the shapes on either side of it. +/// under the port rung's reset, beside the shapes on either side of it. pub fn transport_break_verdict() -> Result<(), String> { let judged = |what: &str, log: &str, green: bool| { match (usb::transport_break_recovered(&Serial::named(what, log)), green) { diff --git a/tests/common/usb.rs b/tests/common/usb.rs index 1f98e47e78..342c452e48 100644 --- a/tests/common/usb.rs +++ b/tests/common/usb.rs @@ -1521,6 +1521,7 @@ pub fn usb_transport_break( abandoned_write_is_taken_offline(test_config, c_bins, rust_bins)?; a_stick_its_reset_moved_carries_on(Moved::SameStick)?; a_stick_its_reset_moved_carries_on(Moved::AfterItsReset)?; + a_stick_its_reset_moved_carries_on(Moved::BeforeItsTestUnitReady)?; a_stick_its_reset_moved_carries_on(Moved::AnotherStick)?; a_stick_its_reset_moved_carries_on(Moved::SlowStick)?; a_stick_its_reset_moved_carries_on(Moved::OwedFlush)?; @@ -1637,6 +1638,11 @@ enum Moved { /// leaves under a USB2 port's reset. The rung's next step fails on an empty /// port, and the rung ends as the stick leaving rather than as a break. AfterItsReset, + /// The stick itself, moved only once the port rung has configured it again + /// (`usb-reset-moves-configured`): the rung's TEST UNIT READY breaks on an + /// empty port, and the rung ends as the stick leaving rather than as a + /// break. + BeforeItsTestUnitReady, /// The same backing under another serial number, which is everything a /// second unit of the same model shares with the first — INQUIRY, /// capacity, USB ids — and the negative control: it must not be adopted. @@ -1679,6 +1685,8 @@ fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { const HELD: &str = "is held empty for the host to move its device (usb-reset-moves)"; const HELD_AFTER: &str = "is held, reset with its device on it, for the host to move the device (usb-reset-moves-after)"; + const HELD_CONFIGURED: &str = + "is held, configured again, for the host to move the device (usb-reset-moves-configured)"; const MOVE_NOW: &str = "usb-reset-moves: move the device now"; const STALLED: &str = "answers slowly (usb-slow-return): its bind is stalled"; const OWED: &str = ", and it left owing a flush of writes it had reported complete, so the \ @@ -1697,6 +1705,7 @@ fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { let params: &'static [&'static str] = match moved { Moved::SameStick | Moved::AnotherStick => &["usb-transport-break", "usb-reset-moves"], Moved::AfterItsReset => &["usb-transport-break", "usb-reset-moves-after"], + Moved::BeforeItsTestUnitReady => &["usb-transport-break", "usb-reset-moves-configured"], Moved::SlowStick => &["usb-transport-break", "usb-reset-moves", "usb-slow-return"], Moved::OwedFlush => &["usb-transport-break-owed", "usb-reset-moves"], Moved::FlushedStick => &["usb-transport-break-flushed", "usb-reset-moves"], @@ -1706,6 +1715,7 @@ fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { let (name, serial) = match moved { Moved::SameStick => ("usb-reset-moves-same.img", qemu::BOOT_STICK_SERIAL), Moved::AfterItsReset => ("usb-reset-moves-after.img", qemu::BOOT_STICK_SERIAL), + Moved::BeforeItsTestUnitReady => ("usb-reset-moves-configured.img", qemu::BOOT_STICK_SERIAL), Moved::AnotherStick => ("usb-reset-moves-another.img", "TOYOS0OTHERSTICK"), Moved::SlowStick => ("usb-reset-moves-slow.img", qemu::BOOT_STICK_SERIAL), Moved::OwedFlush => ("usb-reset-moves-owed.img", qemu::BOOT_STICK_SERIAL), @@ -1748,9 +1758,11 @@ fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { // which may come on either side of it. let ends = |c: &str| { let last = match moved { - Moved::SameStick | Moved::AfterItsReset | Moved::SlowStick | Moved::FlushedStick => { - c.contains(toyos_build::bootlog::REBOOTING) - } + Moved::SameStick + | Moved::AfterItsReset + | Moved::BeforeItsTestUnitReady + | Moved::SlowStick + | Moved::FlushedStick => c.contains(toyos_build::bootlog::REBOOTING), Moved::AnotherStick => c.contains(" did not come back within "), Moved::OwedFlush => c.contains(FLUSH_LOST), Moved::SilentReturn => c.contains(&format!("{WENT_OUT_AGAIN}: it failed")), @@ -1780,16 +1792,28 @@ fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { }; let held = " after this driver reset it; it is held "; let mut left = vec![format!("usb-storage: {under_test} is owed the data of the command that broke")]; - if moved == Moved::AfterItsReset { - left.extend([ + match moved { + Moved::AfterItsReset => left.extend([ HELD_AFTER.to_string(), format!( "usb-storage: {under_test} the port reset was not answered, and port 1 no longer \ holds the device (PORTSC " ), - ]); - } else { - left.push(HELD.to_string()); + ]), + Moved::BeforeItsTestUnitReady => left.extend([ + HELD_CONFIGURED.to_string(), + format!( + "usb-storage: {under_test} transport broke on the port reset's TEST UNIT READY: the \ + port disconnected during the command phase, and port 1 no longer holds the device \ + (PORTSC " + ), + ]), + Moved::SameStick + | Moved::AnotherStick + | Moved::SlowStick + | Moved::OwedFlush + | Moved::FlushedStick + | Moved::SilentReturn => left.push(HELD.to_string()), } left.extend([ // Its port read empty inside the rung, or — when the host's move came @@ -1839,7 +1863,11 @@ fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { }; let came_back = "usb-storage: disk 0 came back on port 3 slot "; match moved { - Moved::SameStick | Moved::AfterItsReset | Moved::SlowStick | Moved::FlushedStick => { + Moved::SameStick + | Moved::AfterItsReset + | Moved::BeforeItsTestUnitReady + | Moved::SlowStick + | Moved::FlushedStick => { if moved == Moved::FlushedStick { let flushed = line_with(&log, AFTER_A_FLUSH)?; if !log.split_once(staged).is_some_and(|(before, _)| before.contains(flushed)) { @@ -1900,6 +1928,9 @@ fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { }, match moved { Moved::AfterItsReset => ", once its reset had been read complete with it there", + Moved::BeforeItsTestUnitReady => { + ", once the rung had configured it again and before its TEST UNIT READY" + } Moved::SlowStick => ", though its bind was stalled past the window", Moved::FlushedStick => { ", owing no flush: it broke after a flush that succeeded over its last write" @@ -2161,7 +2192,6 @@ pub fn transport_break_recovered(kernel: &serial::Serial) -> Result<(), String> &format!("usb-storage: {under_test} is owed the data of the command that broke"), )?; kernel.must_not_say(&format!("usb-storage: {under_test} is offline"))?; - kernel.must_not_say(" did not come back within ")?; if let Ok(left) = kernel.must_say_after(entered, " after this driver reset it; it is held ") { let back = kernel.must_say_after(left, " as the same device (USB ")?; kernel.must_say_after( diff --git a/toyos-xhci/src/ladder.rs b/toyos-xhci/src/ladder.rs index dc9ddc9361..380d59896a 100644 --- a/toyos-xhci/src/ladder.rs +++ b/toyos-xhci/src/ladder.rs @@ -31,8 +31,6 @@ //! again asks the same question of the same device: the next break climbs on. //! Only a completed round trip ends the run. -use crate::portsc::Portsc; - /// One rung of the ladder. #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Debug)] pub enum Rung { @@ -187,20 +185,6 @@ pub const PORT_RESET: [PortStep; 7] = [ /// the 50 ms Linux's hub driver waits, for the firmware that needs it. pub const RESET_RECOVERY_NS: u64 = 50_000_000; -/// Whether a port still holds the device a rung is about: connected, and with -/// no connect change the driver has not consumed, which would be a disconnect -/// (xHCI 1.2 §5.4.8's CSC). -/// -/// **Asked again when a rung ends unverified**, because the reset's completion -/// cannot answer it on a USB2 port: the port detects no disconnect while it -/// drives the reset (§4.19.1.1.2, note 57) and reads Enabled once the reset -/// ends (§4.19.1.1.4), whether or not its device then stays. A device its port -/// no longer holds left: that is no break, no rung above reaches it, and its -/// port's teardown owns what it held (§4.4). -pub fn holds(port: Portsc) -> bool { - port.connected() && !port.connect_changed() -} - /// Whether a failed `step` ends [`Rung::PortReset`]. pub fn ends_the_rung(step: PortStep) -> bool { step != PortStep::Quiesce @@ -376,18 +360,6 @@ mod tests { } } - /// The T14's USB2 port under the port rung's reset: before it, at its - /// completion, and once its SuperSpeed stick had left for the USB3 half of - /// the receptacle. - #[test] - fn a_port_holds_its_device_only_while_it_reads_connected_with_no_change_unconsumed() { - assert!(holds(Portsc::from_raw(0x0000_0e03)), "Enabled at high speed"); - assert!(holds(Portsc::from_raw(0x0020_0e03)), "a reset that ended is no connect change"); - assert!(!holds(Portsc::from_raw(0x0002_02a0)), "Disconnected (§4.19.1.1.2), unconsumed"); - assert!(!holds(Portsc::from_raw(0x0000_02a0)), "Disconnected, its change consumed"); - assert!(!holds(Portsc::from_raw(0x0002_02e1)), "Disabled on a connect since: another connection"); - } - #[test] fn a_device_is_addressed_again_only_as_what_it_was_enumerated_as() { assert_eq!(after_reset(true, true, true, 3, 3), AfterReset::Enumerate); diff --git a/toyos-xhci/src/portsc.rs b/toyos-xhci/src/portsc.rs index d292f33855..bfb3227ed2 100644 --- a/toyos-xhci/src/portsc.rs +++ b/toyos-xhci/src/portsc.rs @@ -110,6 +110,14 @@ impl Portsc { self.0 & CSC != 0 } + /// Whether the port still holds the device the driver last consumed its + /// connect change for: connected, with no connect change since. A device + /// replugged between two looks reads connected again, and only CSC says + /// the one that was here has gone. + pub const fn holds(self) -> bool { + self.connected() && !self.connect_changed() + } + /// Whether reset signalling is still on the wire. /// /// **PR is RW1S and the xHC is what clears it** (§4.19.5), so a port reading @@ -306,6 +314,18 @@ mod tests { assert_ne!(write.raw() & PR, 0); } + /// The T14's USB2 port under the port rung's reset: before it, at its + /// completion, and once its SuperSpeed stick had left for the USB3 half of + /// the receptacle. + #[test] + fn a_port_holds_its_device_only_while_it_reads_connected_with_no_change_unconsumed() { + assert!(Portsc::from_raw(0x0000_0e03).holds(), "Enabled at high speed"); + assert!(Portsc::from_raw(0x0020_0e03).holds(), "a reset that ended is no connect change"); + assert!(!Portsc::from_raw(0x0002_02a0).holds(), "Disconnected (§4.19.1.1.2), unconsumed"); + assert!(!Portsc::from_raw(0x0000_02a0).holds(), "Disconnected, its change consumed"); + assert!(!Portsc::from_raw(0x0002_02e1).holds(), "Disabled on a connect since: another connection"); + } + /// A change flag raised between the read and the write is not cleared by /// it, so the next pass still sees it. #[test] From 989aec4ff32405824b1bae2dcaee16803ef2f6c5 Mon Sep 17 00:00:00 2001 From: japabu Date: Wed, 30 Sep 2026 18:50:05 +0200 Subject: [PATCH 7/8] The leave decision is toyos_xhci::ladder's, and usb_stick_left is the fix's test Review round 5 of #588: the fix's only test was usb_transport_break, which is redlisted and which #639 deletes. Whichever landed first, main kept the fix with nothing that could go red on it. toyos_xhci::ladder now decides what a rung that did not verify leaves: - `Run::unverified(&Unverified, holds)` answers `AfterRung::Left` for any rung, however it ended, on a port that no longer holds its device, and otherwise the next climb with the transfer event the next rung quiesces against. The kernel reads PORTSC once the rung has ended and applies the answer; its `Climbed` enum is replaced by `Result<(), Unverified>`. - `AfterReset::goes_on` is the port rung's decision to go on past its reset. - `a_rung_that_did_not_verify_left_exactly_where_its_port_no_longer_holds_the_device` covers both rungs, every ending, and both port readings, so the review's n1 and c1 narrowings, the arm deleted, and B2's fold of `Left` into a reset that took are crate mutations `cargo test -p toyos-xhci` turns red. usb_stick_left (tests/common/usb.rs) stages the leave on the gate's data stick, which the host unplugs on the hold's cue and never plugs back: the three holds `usb-reset-moves`, `-after` and `-configured`. Each boot judges the leave line, no `is offline`, no `break 2`, and the teardown's `slot N disabled` after the leave; the `usb-reset-moves` and `-configured` boots refuse `Reset Device failed` and `Address Device (after the port reset)` between the hold and the leave. It has no redlist row. The T14 row judged by `transport_break_on_metal` moves onto it; its comment is deleted, since the verdict refuses the class reset it named. usb_transport_break and its redlist row are as on main again: the `AfterItsReset` and `BeforeItsTestUnitReady` boots and the `is offline` refusal leave it, and the actuators only it arms are untouched. Filed: a bind whose device left climbs the ladder at its empty port, the review's two-exits NOTE, which main has too. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016t9wjdQkB8SH7bmfUoiy6L --- ...ck-enumerates-at-high-speed-under-toyos.md | 3 +- ...-left-aims-the-ladder-at-its-empty-port.md | 27 +++ kernel/src/actuator.rs | 10 +- kernel/src/drivers/xhci/wait/msc.rs | 117 ++++----- tests/common/power.rs | 2 +- tests/common/usb.rs | 225 +++++++++++++----- tests/toyos.rs | 11 +- toyos-xhci/src/ladder.rs | 103 ++++++++ 8 files changed, 350 insertions(+), 148 deletions(-) create mode 100644 issues/kernel/a-bind-whose-device-left-aims-the-ladder-at-its-empty-port.md diff --git a/issues/hardware/the-t14s-superspeed-stick-enumerates-at-high-speed-under-toyos.md b/issues/hardware/the-t14s-superspeed-stick-enumerates-at-high-speed-under-toyos.md index e13b2a4026..c1da878b32 100644 --- a/issues/hardware/the-t14s-superspeed-stick-enumerates-at-high-speed-under-toyos.md +++ b/issues/hardware/the-t14s-superspeed-stick-enumerates-at-high-speed-under-toyos.md @@ -39,8 +39,7 @@ whether the boot scan's own reset of port 1 is what keeps the stick there, whether the USB3 port reads connected at the scan, and why the port rung's reset moved the stick where the scan's reset of the same port 0.8 s earlier did not. xHCI 1.2 says why the USB2 port cannot see the stick leave, not why -the stick leaves: that is the USB 3.x specification's device-side rule, which -nobody here has read. +the stick leaves: that is the USB 3.x specification's device-side rule. ## Exit condition diff --git a/issues/kernel/a-bind-whose-device-left-aims-the-ladder-at-its-empty-port.md b/issues/kernel/a-bind-whose-device-left-aims-the-ladder-at-its-empty-port.md new file mode 100644 index 0000000000..319eab309b --- /dev/null +++ b/issues/kernel/a-bind-whose-device-left-aims-the-ladder-at-its-empty-port.md @@ -0,0 +1,27 @@ +--- +status: open +kind: defect +opened: 2026-09-30 +--- + +# A bind whose device left aims the ladder at its empty port + +Derived from the code, not staged. + +`kernel/src/drivers/xhci/wait/msc.rs` has two exits for a device that left its +port. `scsi()` answers a round trip that broke as `Broke::Gone` itself: it +sends no recovery and leaves the disk to its port's teardown. `bring_up` does +not. Its TEST UNIT READY hands every break to `climb_until_in_step`, a `Gone` +one included, with `Left::Elsewhere`. The climb enters at the class reset, whose +steps are aimed at a port that reads empty. Only once that rung has ended does +the climb read the port and find the device gone. + +The class rung's bound is spent on a device that is not on the bus, and a +device that left during its bind is handed to the teardown by another path +than one that left during a command. `main` at 3c24b6edb has the same shape: +its bind climbs a `Gone` break too, and the port rung's reset is where it +first reads the port empty. + +**Exit**: the port is read once, at the climb's entry as well as after each +rung, so a break whose device left ends there before any rung. `scsi()`'s +`Broke::Gone` arm then goes, and a device that left is one exit. diff --git a/kernel/src/actuator.rs b/kernel/src/actuator.rs index 502e5453db..92fc854a73 100644 --- a/kernel/src/actuator.rs +++ b/kernel/src/actuator.rs @@ -135,20 +135,20 @@ actuators! { /// ladder, once: a device that answers nothing on any rung. usb_reset_break = "usb-reset-break"; - /// Hold the port rung's first reset, once, until the port reads empty, so - /// the host can move the device to another port. See - /// `xhci::msc::reset_moves`; judged by `usb_transport_break`. + /// Hold the port rung's first reset, once, until the port reads empty. See + /// `xhci::msc::reset_moves`; judged by `usb_transport_break` and + /// `usb_stick_left`. usb_reset_moves = "usb-reset-moves"; /// The same hold, once the reset's completion has been read with the /// device on the port: a device that leaves under a USB2 port's reset. - /// See `xhci::msc::reset_moves`; judged by `usb_transport_break`. + /// See `xhci::msc::reset_moves`; judged by `usb_stick_left`. usb_reset_moves_after = "usb-reset-moves-after"; /// The same hold, once the port rung has configured the device again and /// before its TEST UNIT READY: a device that leaves after every step of /// the rung was answered. See `xhci::msc::reset_moves`; judged by - /// `usb_transport_break`. + /// `usb_stick_left`. usb_reset_moves_configured = "usb-reset-moves-configured"; /// `usb-transport-break`'s break, on the first WRITE(10) that goes out diff --git a/kernel/src/drivers/xhci/wait/msc.rs b/kernel/src/drivers/xhci/wait/msc.rs index a083a58799..98062fcc83 100644 --- a/kernel/src/drivers/xhci/wait/msc.rs +++ b/kernel/src/drivers/xhci/wait/msc.rs @@ -27,7 +27,7 @@ use toyos_xhci::configure::{self, BulkEndpoint}; use toyos_xhci::flush::Debt; use toyos_xhci::identity::{self, Identity, Serial, UsbId}; use toyos_xhci::job::CC_SUCCESS; -use toyos_xhci::ladder::{self, AfterReset, Left, PortStep, Run, Rung}; +use toyos_xhci::ladder::{self, AfterReset, AfterRung, Left, PortStep, Run, Rung}; use toyos_xhci::port; use toyos_xhci::reset_recovery::{self, Answered, GaveUp, Look, Pipe, Quiescing, SlotGoes, Step}; use toyos_xhci::scsi::{self, BringUp, Cdb, Fail, Flushed, Geometry, Heard, Moved, Printable, Reply}; @@ -40,6 +40,9 @@ type DataPhase = Option>; /// Why a round trip broke, with this driver's reason for a silence. type Broke = bot::Broke; +/// How a rung ended when it did not verify; a rung that did is `Ok(())`. +type Unverified = ladder::Unverified; + /// Wall-clock budget on bring-up's ready attempts: bounds when [`bring_up`] /// stops *starting* attempts, not the one already running. const READY_BUDGET: Budget = Budget::of( @@ -276,37 +279,20 @@ impl core::fmt::Display for Told<'_> { } } -/// A rung that did not bring its device back in step, as its line says it: -/// its TEST UNIT READY broke this way, or a step before it was not answered. -struct Unverified<'a> { - rung: Rung, - broke: Option<&'a Broke>, -} +/// A rung that did not bring its device back in step, as its line says it. +struct Ended<'a>(Rung, &'a Unverified); -impl core::fmt::Display for Unverified<'_> { +impl core::fmt::Display for Ended<'_> { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - match self.broke { - Some(why) => { - write!(f, "transport broke on the {}'s TEST UNIT READY: {}", self.rung.named(), Told(why)) + match self.1 { + Unverified::OutOfStep(why) => { + write!(f, "transport broke on the {}'s TEST UNIT READY: {}", self.0.named(), Told(why)) } - None => write!(f, "the {} was not answered", self.rung.named()), + Unverified::Failed => write!(f, "the {} was not answered", self.0.named()), } } } -/// How one rung of the ladder ended. -enum Climbed { - /// The device answered the rung's TEST UNIT READY under that command's own - /// tag. - InStep, - /// Everything before the TEST UNIT READY was answered, and it broke this - /// way: a break like the one recovered from, and counted as one. - OutOfStep(Broke), - /// A step before the TEST UNIT READY was not answered, and said so; the - /// device was asked nothing after it. - Failed, -} - /// Abandon one bulk transfer without waiting, once per boot, on the first /// WRITE(10): only the wait is skipped, so recovery runs against a real /// endpoint state — staged since nothing on the host side leaves one in flight. @@ -539,9 +525,8 @@ pub(in crate::drivers::xhci) mod reset_break { const RECOVERING: &str = "recovering"; /// Hold the port rung, once, until its port reads empty: QEMU cannot move a -/// device off its port on a reset, so the host takes it off and plugs the same -/// backing in on another port. `usb-reset-moves` holds before the reset's -/// completion is read, so the rung reads the port empty; +/// device off its port on a reset, so the host takes it off. `usb-reset-moves` +/// holds before the reset's completion is read, so the rung reads the port empty; /// `usb-reset-moves-after` once it has been read with the device on the port, /// as a USB2 port reads a device that leaves under its reset; /// `usb-reset-moves-configured` once the rung has configured the device again, @@ -1093,10 +1078,11 @@ impl XhciController { in_step } - fn climb(&mut self, dev: &mut MscDevice, mut broke: Option<(Pipe, u32)>, mut left: Left) -> bool { + fn climb(&mut self, dev: &mut MscDevice, mut broke: Option<(Pipe, u32)>, left: Left) -> bool { let slot = self.slot(dev.slot_id); + let mut climb = dev.run.broke(left); loop { - let ladder::Climb { rung, skips_class_reset } = dev.run.broke(left); + let ladder::Climb { rung, skips_class_reset } = climb; if skips_class_reset { log!("usb-storage: {slot} is owed the data of the command that broke, so nothing \ can be asked of it on the Bulk-Out: its port is reset with no class reset \ @@ -1113,36 +1099,31 @@ impl XhciController { return false; } }; - let out_of_step = match climbed { - Climbed::InStep => { - self.after_break.took(rung); - return true; - } - Climbed::OutOfStep(why) => Some(why), - Climbed::Failed => None, + let Err(unverified) = climbed else { + self.after_break.took(rung); + return true; }; - let ended = Unverified { rung, broke: out_of_step.as_ref() }; - // A device its port no longer holds left, which is no break and - // which no rung above reaches: its port's teardown owns what it - // held. Asked of the port here, since a USB2 port detects no - // disconnect while it drives a reset (xHCI 1.2 §4.19.1.1.2, note - // 57) and reads Enabled once the reset ends (§4.19.1.1.4). + // Asked of the port once the rung has ended, since a USB2 port + // detects no disconnect while it drives a reset (xHCI 1.2 + // §4.19.1.1.2, note 57) and reads Enabled once the reset ends + // (§4.19.1.1.4). let port = self.read_portsc(dev.port_idx); - if !port.holds() { - log!("usb-storage: {slot} {ended}, and port {} no longer holds the device (PORTSC \ - {:#010x}): its port's teardown takes it from here", - u32::from(dev.port_idx) + 1, port.raw()); - dev.failed = true; - dev.left = true; - return false; + let ended = Ended(rung, &unverified); + match dev.run.unverified(&unverified, port.holds()) { + AfterRung::Left => { + log!("usb-storage: {slot} {ended}, and port {} no longer holds the device \ + (PORTSC {:#010x}): its port's teardown takes it from here", + u32::from(dev.port_idx) + 1, port.raw()); + dev.failed = true; + dev.left = true; + return false; + } + AfterRung::Climbs { climb: next, broke: event } => { + log!("usb-storage: {slot} {ended}; break {} of {MAX_TRANSPORT_BREAKS} running", + dev.run.breaks()); + (climb, broke) = (next, event); + } } - log!("usb-storage: {slot} {ended}; break {} of {MAX_TRANSPORT_BREAKS} running", - dev.run.breaks().saturating_add(1)); - // After a step that was not answered the event is spent: the rung - // has commanded the pair since, and only the fields speak for it now. - broke = out_of_step.as_ref().and_then(Broke::event); - // A rung's own TEST UNIT READY has no data phase to be left in. - left = Left::Elsewhere; } } @@ -1286,7 +1267,7 @@ impl XhciController { /// their order are `ladder::PORT_RESET`'s; this takes them, one blocking /// command or control transfer at a time, and ends on the device's answer /// to TEST UNIT READY. - fn port_reset_recovery(&mut self, dev: &mut MscDevice, broke: Option<(Pipe, u32)>) -> Climbed { + fn port_reset_recovery(&mut self, dev: &mut MscDevice, broke: Option<(Pipe, u32)>) -> Result<(), Unverified> { let slot = self.slot(dev.slot_id); for step in ladder::PORT_RESET { let took = match step { @@ -1294,7 +1275,7 @@ impl XhciController { self.quiesce_bulk_pair(dev, broke, "stopping it before its port is reset") } // `reset_port` has said which way it did not. - PortStep::Reset => self.reset_port(dev, RECOVERING) == AfterReset::Enumerate, + PortStep::Reset => self.reset_port(dev, RECOVERING).goes_on(), PortStep::Settle => { let _ = crate::clock::settles( self.after_break @@ -1330,7 +1311,7 @@ impl XhciController { ), }; if !took && ladder::ends_the_rung(step) { - return Climbed::Failed; + return Err(Unverified::Failed); } } #[cfg(feature = "boot-actuators")] @@ -1342,9 +1323,9 @@ impl XhciController { log!("usb-storage: {slot} the port reset took: addressed and configured again, the \ device answered TEST UNIT READY under its own tag {:#x}", dev.tag); self.take_held_sense(dev, answer); - Climbed::InStep + Ok(()) } - Err(why) => Climbed::OutOfStep(why), + Err(why) => Err(Unverified::OutOfStep(why)), } } @@ -1586,15 +1567,15 @@ impl XhciController { /// A command that did not take ends it, since the requests after it assume /// both endpoints are off their transfers; a request that did not is /// followed by the rest, so the device is left with both pipes cleared - /// whatever the next rung then does with it. Either is [`Climbed::Failed`]. + /// whatever the next rung then does with it. Either is [`ladder::Unverified::Failed`]. /// /// **Then the device is asked, and only its answer says the recovery /// took**: TEST UNIT READY, whose status must carry that command's own tag. /// /// `broke` is the transfer event that ended the round trip, where one did. - fn reset_recovery(&mut self, dev: &mut MscDevice, broke: Option<(Pipe, u32)>) -> Climbed { + fn reset_recovery(&mut self, dev: &mut MscDevice, broke: Option<(Pipe, u32)>) -> Result<(), Unverified> { if !self.quiesce_bulk_pair(dev, broke, "recovering") { - return Climbed::Failed; + return Err(Unverified::Failed); } let slot = self.slot(dev.slot_id); let mut recovered = true; @@ -1624,16 +1605,16 @@ impl XhciController { } } if !recovered { - return Climbed::Failed; + return Err(Unverified::Failed); } match self.bot(dev, &Cdb::TEST_UNIT_READY, None, Asks::Verification(Rung::ClassReset)) { Ok(answer) => { log!("usb-storage: {slot} Reset Recovery took: the device answered TEST UNIT \ READY under its own tag {:#x}", dev.tag); self.take_held_sense(dev, answer); - Climbed::InStep + Ok(()) } - Err(why) => Climbed::OutOfStep(why), + Err(why) => Err(Unverified::OutOfStep(why)), } } diff --git a/tests/common/power.rs b/tests/common/power.rs index 03421a895e..3e2aa24e99 100644 --- a/tests/common/power.rs +++ b/tests/common/power.rs @@ -1168,7 +1168,7 @@ pub fn blackbox_done_chain( Ok(()) } -/// The boot the T14 takes for `usb_transport_break`, under QEMU: the break is +/// The boot the T14 takes for `usb_stick_left`, under QEMU: the break is /// staged on the stick the machine booted from, and the page the next pass /// reads carries the transport's recovery whatever the log volume got. /// diff --git a/tests/common/usb.rs b/tests/common/usb.rs index d857cc7e74..fbbaaa641f 100644 --- a/tests/common/usb.rs +++ b/tests/common/usb.rs @@ -1519,8 +1519,6 @@ pub fn usb_transport_break( transport_gives_up(test_config, c_bins, rust_bins)?; abandoned_write_is_taken_offline(test_config, c_bins, rust_bins)?; a_stick_its_reset_moved_carries_on(Moved::SameStick)?; - a_stick_its_reset_moved_carries_on(Moved::AfterItsReset)?; - a_stick_its_reset_moved_carries_on(Moved::BeforeItsTestUnitReady)?; a_stick_its_reset_moved_carries_on(Moved::AnotherStick)?; a_stick_its_reset_moved_carries_on(Moved::SlowStick)?; a_stick_its_reset_moved_carries_on(Moved::OwedFlush)?; @@ -1632,16 +1630,6 @@ enum Moved { /// reset can move a stick from the USB2 half of its receptacle to the USB3 /// half. SameStick, - /// The stick itself, moved only once its port's reset has been read - /// complete with it on the port (`usb-reset-moves-after`): a device that - /// leaves under a USB2 port's reset. The rung's next step fails on an empty - /// port, and the rung ends as the stick leaving rather than as a break. - AfterItsReset, - /// The stick itself, moved only once the port rung has configured it again - /// (`usb-reset-moves-configured`): the rung's TEST UNIT READY breaks on an - /// empty port, and the rung ends as the stick leaving rather than as a - /// break. - BeforeItsTestUnitReady, /// The same backing under another serial number, which is everything a /// second unit of the same model shares with the first — INQUIRY, /// capacity, USB ids — and the negative control: it must not be adopted. @@ -1682,10 +1670,6 @@ enum Moved { /// the bind is another CPU's, read off the kernel's own stamps. fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { const HELD: &str = "is held empty for the host to move its device (usb-reset-moves)"; - const HELD_AFTER: &str = - "is held, reset with its device on it, for the host to move the device (usb-reset-moves-after)"; - const HELD_CONFIGURED: &str = - "is held, configured again, for the host to move the device (usb-reset-moves-configured)"; const MOVE_NOW: &str = "usb-reset-moves: move the device now"; const STALLED: &str = "answers slowly (usb-slow-return): its bind is stalled"; const OWED: &str = ", and it left owing a flush of writes it had reported complete, so the \ @@ -1700,8 +1684,6 @@ fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { out again on it"; let params: &'static [&'static str] = match moved { Moved::SameStick | Moved::AnotherStick => &["usb-transport-break", "usb-reset-moves"], - Moved::AfterItsReset => &["usb-transport-break", "usb-reset-moves-after"], - Moved::BeforeItsTestUnitReady => &["usb-transport-break", "usb-reset-moves-configured"], Moved::SlowStick => &["usb-transport-break", "usb-reset-moves", "usb-slow-return"], Moved::OwedFlush => &["usb-transport-break-owed", "usb-reset-moves"], Moved::FlushedStick => &["usb-transport-break-flushed", "usb-reset-moves"], @@ -1710,8 +1692,6 @@ fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { let case = super::compile::repo_root().join("tests/jobcase"); let (name, serial) = match moved { Moved::SameStick => ("usb-reset-moves-same.img", qemu::BOOT_STICK_SERIAL), - Moved::AfterItsReset => ("usb-reset-moves-after.img", qemu::BOOT_STICK_SERIAL), - Moved::BeforeItsTestUnitReady => ("usb-reset-moves-configured.img", qemu::BOOT_STICK_SERIAL), Moved::AnotherStick => ("usb-reset-moves-another.img", "TOYOS0OTHERSTICK"), Moved::SlowStick => ("usb-reset-moves-slow.img", qemu::BOOT_STICK_SERIAL), Moved::OwedFlush => ("usb-reset-moves-owed.img", qemu::BOOT_STICK_SERIAL), @@ -1764,11 +1744,9 @@ fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { // which may come on either side of it. let ends = |c: &str| { let last = match moved { - Moved::SameStick - | Moved::AfterItsReset - | Moved::BeforeItsTestUnitReady - | Moved::SlowStick - | Moved::FlushedStick => c.contains(toyos_build::bootlog::REBOOTING), + Moved::SameStick | Moved::SlowStick | Moved::FlushedStick => { + c.contains(toyos_build::bootlog::REBOOTING) + } Moved::AnotherStick => c.contains(" did not come back within "), Moved::OwedFlush => c.contains(FLUSH_LOST), Moved::SilentReturn => c.contains(&format!("{WENT_OUT_AGAIN}: it failed")), @@ -1796,39 +1774,16 @@ fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { } Ok(()) }; - let held = " after this driver reset it; it is held "; - let mut left = vec![format!("usb-storage: {under_test} is owed the data of the command that broke")]; - match moved { - Moved::AfterItsReset => left.extend([ - HELD_AFTER.to_string(), - format!( - "usb-storage: {under_test} the port reset was not answered, and port 1 no longer \ - holds the device (PORTSC " - ), - ]), - Moved::BeforeItsTestUnitReady => left.extend([ - HELD_CONFIGURED.to_string(), - format!( - "usb-storage: {under_test} transport broke on the port reset's TEST UNIT READY: the \ - port disconnected during the command phase, and port 1 no longer holds the device \ - (PORTSC " - ), - ]), - Moved::SameStick - | Moved::AnotherStick - | Moved::SlowStick - | Moved::OwedFlush - | Moved::FlushedStick - | Moved::SilentReturn => left.push(HELD.to_string()), - } - left.extend([ + let left = [ + format!("usb-storage: {under_test} is owed the data of the command that broke"), + HELD.to_string(), // Its port read empty inside the rung, or — when the host's move came // after the rung's bound and the reset verified — disconnected at the // next look: run 79's shape and run 74's, both a device that left // under a reset of this driver's. "usb-storage: disk 0 left port 1 (".to_string(), - held.to_string(), - ]); + " after this driver reset it; it is held ".to_string(), + ]; let inside_the_rung = log.contains("usb-storage: disk 0 left port 1 (its port read empty)"); let back = [ "xHCI: port 3 connected".to_string(), @@ -1869,11 +1824,7 @@ fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { }; let came_back = "usb-storage: disk 0 came back on port 3 slot "; match moved { - Moved::SameStick - | Moved::AfterItsReset - | Moved::BeforeItsTestUnitReady - | Moved::SlowStick - | Moved::FlushedStick => { + Moved::SameStick | Moved::SlowStick | Moved::FlushedStick => { if moved == Moved::FlushedStick { let flushed = line_with(&log, AFTER_A_FLUSH)?; if !log.split_once(staged).is_some_and(|(before, _)| before.contains(flushed)) { @@ -1882,7 +1833,6 @@ fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { } // First, and the debt first of all, so a debt the stick came back // with is named as one. - let offline = format!("usb-storage: {under_test} is offline"); for never in [ OWED, FLUSH_LOST, @@ -1890,7 +1840,6 @@ fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { " did not come back within ", "disk 1 ready", " is not disk 0 come back", - offline.as_str(), ] { if let Some(line) = log.lines().find(|l| l.contains(never)) { return Err(format!("{moved:?}: {line:?} of a stick that came back as itself\n{log}")); @@ -1909,11 +1858,11 @@ fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { // stall, or every CPU inside a call on the held disk, so no CPU // took the pass that binds — and the write was asked again. let shape = if moved != Moved::SlowStick && held_end.ends_with(": it completed") { - in_order(&[held.to_string(), came_back.to_string(), held_end.to_string()])?; + in_order(&[left[3].clone(), came_back.to_string(), held_end.to_string()])?; held_call(came_back)?; "the write that waited went out again on it" } else if held_end.contains(STILL_HELD) { - in_order(&[held.to_string(), held_end.to_string()])?; + in_order(&[left[3].clone(), held_end.to_string()])?; held_call(if moved == Moved::SlowStick { STALLED } else { came_back })?; "the call that waited ended on its bound and the write was asked again" } else { @@ -1933,10 +1882,6 @@ fn a_stick_its_reset_moved_carries_on(moved: Moved) -> Result<(), String> { "after the rung's reset verified" }, match moved { - Moved::AfterItsReset => ", once its reset had been read complete with it there", - Moved::BeforeItsTestUnitReady => { - ", once the rung had configured it again and before its TEST UNIT READY" - } Moved::SlowStick => ", though its bind was stalled past the window", Moved::FlushedStick => { ", owing no flush: it broke after a flush that succeeded over its last write" @@ -2167,6 +2112,154 @@ fn no_command_was_refused(log: &str) -> Result<(), String> { Ok(()) } +/// Where `reset_moves` holds the port rung for the host to unplug the stick. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +enum Held { + /// `usb-reset-moves`, before the reset's completion is read: the reset + /// reads the port empty. + BeforeItsCompletion, + /// `usb-reset-moves-after`, once the completion has been read with the + /// stick on the port, as a USB2 port reads a device that leaves under its + /// reset: the rung's next step fails on the empty port. + AfterItsCompletion, + /// `usb-reset-moves-configured`, once the rung has configured the stick + /// again: its TEST UNIT READY breaks on the empty port. + BeforeItsTestUnitReady, +} + +/// The gate's data stick, owed a WRITE's data by the staged break, leaves its +/// port inside the port rung the break entered and is not plugged back: the +/// rung ends as the stick leaving. No rung takes it offline, no second break is +/// counted, and its port's teardown gives the slot back. +/// +/// **QEMU cannot take a device off its port on a reset**, so `reset_moves` +/// holds the rung once, at the place [`Held`] names, until the port reads +/// empty, and the host unplugs the stick on the cue the hold writes. +pub fn usb_stick_left( + test_config: &Path, + c_bins: &[(String, Vec)], + rust_bins: &[(String, Vec)], +) -> Result<(), String> { + for held in [Held::BeforeItsCompletion, Held::AfterItsCompletion, Held::BeforeItsTestUnitReady] { + a_stick_that_left_under_its_rung(test_config, c_bins, rust_bins, held)?; + } + Ok(()) +} + +fn a_stick_that_left_under_its_rung( + test_config: &Path, + c_bins: &[(String, Vec)], + rust_bins: &[(String, Vec)], + held: Held, +) -> Result<(), String> { + const MOVE_NOW: &str = "usb-reset-moves: move the device now"; + let (params, hold, ended): (&'static [&'static str], &str, &str) = match held { + Held::BeforeItsCompletion => ( + &["usb-storage-gate", "usb-transport-break", "usb-reset-moves"], + "is held empty for the host to move its device (usb-reset-moves)", + "the port reset was not answered", + ), + Held::AfterItsCompletion => ( + &["usb-storage-gate", "usb-transport-break", "usb-reset-moves-after"], + "is held, reset with its device on it, for the host to move the device \ + (usb-reset-moves-after)", + "the port reset was not answered", + ), + Held::BeforeItsTestUnitReady => ( + &["usb-storage-gate", "usb-transport-break", "usb-reset-moves-configured"], + "is held, configured again, for the host to move the device \ + (usb-reset-moves-configured)", + "transport broke on the port reset's TEST UNIT READY: the port disconnected during \ + the command phase", + ), + }; + let (bytes, _) = Profile::UsbDisk.usb_disk().expect("UsbDisk declares a disk"); + let image = test_dir().join(format!("usb-stick-left-{held:?}.img")); + stage(&image, bytes); + let mut qemu = QemuInstance::boot_with_options( + test_config, + c_bins, + rust_bins, + BootOptions { + profile: Profile::UsbDisk, + qmp: true, + kernel_params: params, + usb_images: vec![image.clone()], + // The hold is inside the boot's USB gate, before any ready marker. + ready_marker: toyos_build::bootlog::LOADER_LAST_LINE, + ..Default::default() + }, + ); + let mut log = qemu.boot_log().to_string(); + // On the cue the staging writes to the console itself: the record above it + // waits for `klogd`, which may not run while the rung holds its CPU. + log.push_str(&qemu.drain_until(Duration::from_secs(60), |l| l.contains(MOVE_NOW))); + if !log.contains(MOVE_NOW) { + return Err(format!("{held:?}: the port rung was never held for the host\n{log}")); + } + let mut devices = qemu::QmpDevices::open(qemu.qmp_socket()); + devices.del(&qemu::usb_device_id(0)); + drop(devices); + // Whichever way the rung ended, a slot goes back after the hold: the + // teardown's, or the last rung's. + qemu::await_guest(&mut qemu, &mut log, "the boot to complete and a slot to go back", |c| { + c.contains("Boot: complete") + && c.split_once(hold).is_some_and(|(_, after)| { + after.lines().any(|l| l.contains("xHCI: slot ") && l.ends_with(" disabled")) + }) + }) + .map_err(|why| format!("{held:?}: {why}\n{log}"))?; + drop(qemu); + let _ = std::fs::remove_file(&image); + + let kernel = serial::Serial::named(&format!("{held:?} boot console"), log.as_str()); + let staged = kernel.must_say( + "transport broke on SCSI 0x2a: a staged break skipped the data phase wait; break 1 of ", + )?; + let under_test = broke_on(staged)?; + let entered = kernel.must_say_after( + staged, + &format!("usb-storage: {under_test} is owed the data of the command that broke"), + )?; + let held_there = kernel.must_say_after(entered, hold)?; + let port = held_there + .split_once(&format!("xHCI: {under_test} port ")) + .and_then(|(_, rest)| rest.split_once(' ')) + .map(|(port, _)| port) + .ok_or_else(|| format!("{held:?}: {held_there:?} holds no port of {under_test}\n{log}"))?; + let left = kernel.must_say_after( + held_there, + &format!( + "usb-storage: {under_test} {ended}, and port {port} no longer holds the device (PORTSC " + ), + )?; + // Nothing is sent to the empty port once the reset is read: the reset + // that read it so ends the rung, and so does the TEST UNIT READY that + // met it. + if held != Held::AfterItsCompletion { + let (_, from_the_hold) = log.split_once(held_there).expect("the line came from this text"); + let (between, _) = from_the_hold.split_once(left).expect("the leave follows the hold"); + for sent in ["Reset Device failed", "Address Device (after the port reset)"] { + if let Some(line) = between.lines().find(|l| l.contains(sent)) { + return Err(format!("{held:?}: {line:?} between the hold and the leave\n{log}")); + } + } + } + let slot = under_test.rsplit(' ').next().expect("a slot id ends the name"); + let gone_back = kernel.must_say_after(left, &format!("xHCI: slot {slot} disabled"))?; + kernel.must_not_say(&format!("usb-storage: {under_test} is offline"))?; + if let Some(line) = log + .lines() + .find(|l| l.contains(&format!("usb-storage: {under_test} ")) && l.contains(" break 2 of ")) + { + return Err(format!("{held:?}: {line:?}: the stick leaving was counted as a break\n{log}")); + } + kernel.must_be_clean()?; + eprintln!(" [usb] {held:?}: {left}"); + eprintln!(" [usb] {held:?}: {gone_back}"); + Ok(()) +} + /// The staged break on a real stick: the transfer abandoned on the boot stick's /// first WRITE(10) is recovered, the write completes, the disk keeps its /// number, and the boot goes on to the deliberate reboot that ends its chain. diff --git a/tests/toyos.rs b/tests/toyos.rs index 144cadf83d..ac261b0bb2 100644 --- a/tests/toyos.rs +++ b/tests/toyos.rs @@ -1203,6 +1203,9 @@ const MACHINE_TESTS: &[(&str, Sched)] = &[ // had — its own doc says one break under KVM and two under TCG off the // same tree, which is the race timer-anchored, not a margin, describes. ("usb_transport_break", Sched::Serial), + // The host's unplug has to land inside the port rung's bound, which the + // hold spends waiting for it. + ("usb_stick_left", Sched::Serial), ("xhci_full_speed_device", Sched::Parallel), ("xhci_superspeed_ports", Sched::Parallel), // `xhci_flap` is the one that genuinely races the host against the guest: @@ -1852,12 +1855,7 @@ const METAL: &[(&str, metal::Metal)] = &[ }, ), ( - // Its own boot: the first WRITE(10) the boot stick takes is abandoned - // mid-flight, and what is judged is the one thing QEMU's `usb-storage` - // cannot answer — whether a device holding a toggle, a sequence number - // and half a command comes back from the class's Reset Recovery on the - // machine's own controller. - "usb_transport_break", + "usb_stick_left", metal::Metal::Runs { arms: &[metal::once("usbbreak", "tests/jobcase", &["usb-transport-break"], &[])], judge: |b| usb::transport_break_on_metal(&b[0].kernel(), &b[0].after_the_reset()?), @@ -10190,6 +10188,7 @@ fn run_machine_test( "xhci_slow_connect" => usb::xhci_slow_connect(test_config, c_bins, rust_bins), "xhci_portsc_rw1c" => usb::xhci_portsc_rw1c(test_config, c_bins, rust_bins), "usb_transport_break" => usb::usb_transport_break(test_config, c_bins, rust_bins), + "usb_stick_left" => usb::usb_stick_left(test_config, c_bins, rust_bins), "xhci_full_speed_device" => { usb::xhci_full_speed_device(test_config, c_bins, rust_bins) } diff --git a/toyos-xhci/src/ladder.rs b/toyos-xhci/src/ladder.rs index 380d59896a..a943d7c91a 100644 --- a/toyos-xhci/src/ladder.rs +++ b/toyos-xhci/src/ladder.rs @@ -140,6 +140,50 @@ impl Run { pub fn over(&mut self) -> u8 { core::mem::replace(self, Self::NONE).breaks } + + /// The rung this run last climbed ended `ended`, below [`Rung::Offline`], + /// on a port that `holds` its device or not (`Portsc::holds`, read once + /// the rung has ended). + /// + /// **A device its port no longer holds left, whichever rung it was and + /// however the rung ended.** That is no break, and no rung above reaches + /// the device: its port's teardown owns what it held. Otherwise the rung + /// is the next break. + pub fn unverified(&mut self, ended: &Unverified, holds: bool) -> AfterRung { + if !holds { + return AfterRung::Left; + } + // After a step that was not answered the event is spent: the rung has + // commanded the pair since, and only the fields speak for it now. + let broke = match ended { + Unverified::OutOfStep(why) => why.event(), + Unverified::Failed => None, + }; + // A rung's own TEST UNIT READY has no data phase to be left in. + AfterRung::Climbs { climb: self.broke(Left::Elsewhere), broke } + } +} + +/// How a rung below [`Rung::Offline`] ended without bringing its device back +/// in step; `W` is the driver's reason for a silence. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum Unverified { + /// Every step before its TEST UNIT READY was answered, and that broke this + /// way: a break like the one recovered from. + OutOfStep(crate::bot::Broke), + /// A step before its TEST UNIT READY was not answered; the device was + /// asked nothing after it. + Failed, +} + +/// What a climb does after a rung that did not verify ([`Run::unverified`]). +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum AfterRung { + /// The device left. + Left, + /// The next break: the rung it climbs, and the transfer event that rung + /// quiesces against, where one ended the round trip. + Climbs { climb: Climb, broke: Option<(crate::reset_recovery::Pipe, u32)> }, } /// One step of [`Rung::PortReset`], in the order taken. @@ -213,6 +257,13 @@ impl AfterReset { pub fn finished(self) -> bool { !matches!(self, Self::NeverFinished | Self::Left) } + + /// Whether the port rung goes on past its reset: only for the device it + /// was enumerated as, on its port. Any other reading ends the rung, and + /// nothing is sent to a port that reads empty. + pub fn goes_on(self) -> bool { + self == Self::Enumerate + } } impl core::fmt::Display for AfterReset { @@ -307,6 +358,49 @@ mod tests { assert!(!run.broke(Left::OwedDataOut).skips_class_reset, "the class reset was climbed"); } + /// Every run that reaches a rung below [`Rung::Offline`], each way that + /// rung can end unverified: its device left exactly where its port no + /// longer holds it, which is no break; otherwise the rung is the next + /// break, and the next rung quiesces against the event that ended the + /// TEST UNIT READY, where one did. + #[test] + fn a_rung_that_did_not_verify_left_exactly_where_its_port_no_longer_holds_the_device() { + use crate::bot::{Broke, Phase}; + use crate::reset_recovery::Pipe; + let stalled = Broke::<()>::Code { phase: Phase::Command, code: 6, pipe: Pipe::Out }; + let endings = [ + (Unverified::Failed, None), + (Unverified::OutOfStep(stalled), Some((Pipe::Out, 6))), + (Unverified::OutOfStep(Broke::Gone { phase: Phase::Command }), None), + (Unverified::OutOfStep(Broke::Silence { phase: Phase::Status, why: () }), None), + ]; + let runs: [(&[Left], Rung); 3] = [ + (&[Left::Elsewhere], Rung::ClassReset), + (&[Left::Elsewhere, Left::Elsewhere], Rung::PortReset), + (&[Left::OwedDataOut], Rung::PortReset), + ]; + for (breaks, rung) in runs { + let mut at = Run::NONE; + for left in breaks { + at.broke(*left); + } + assert_eq!(at.climbed, Some(rung), "{breaks:?}"); + for (ended, event) in endings { + let mut run = at; + assert_eq!(run.unverified(&ended, false), AfterRung::Left, "{rung:?} {ended:?}"); + assert_eq!(run, at, "a device that left is no break: {rung:?} {ended:?}"); + let mut next = at; + let climb = next.broke(Left::Elsewhere); + assert_eq!( + run.unverified(&ended, true), + AfterRung::Climbs { climb, broke: event }, + "{rung:?} {ended:?}" + ); + assert_eq!(run, next, "{rung:?} {ended:?}"); + } + } + } + #[test] fn a_break_elsewhere_climbs_every_rung() { assert_eq!(next(None, Left::Elsewhere), Rung::ClassReset); @@ -374,5 +468,14 @@ mod tests { for left in [AfterReset::Enumerate, AfterReset::NotEnabled, AfterReset::SpeedChanged { was: 3, now: 4 }] { assert!(left.finished(), "{left:?}"); } + assert!(AfterReset::Enumerate.goes_on()); + for after in [ + AfterReset::NeverFinished, + AfterReset::Left, + AfterReset::NotEnabled, + AfterReset::SpeedChanged { was: 3, now: 4 }, + ] { + assert!(!after.goes_on(), "{after:?}"); + } } } From d1471dfd075346f7627b36c29adf606a5ddb52d9 Mon Sep 17 00:00:00 2001 From: japabu Date: Wed, 30 Sep 2026 22:59:48 +0200 Subject: [PATCH 8/8] Record the T14's usbbreak boot at 989aec4ff The `--metal usb_stick_left` run at 989aec4ff passed and recorded three new numbers for the `usbbreak` boot: complete_ms 1160, panel_max_us 3905, panel_us 21851. On `main` the redlist keeps that boot from running; this branch is what runs it, so its records land here. The same run's `boot.testcases` rows are not this branch's and are left out. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016t9wjdQkB8SH7bmfUoiy6L --- tests/metal/lenovo-20w0003amz.toml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/metal/lenovo-20w0003amz.toml b/tests/metal/lenovo-20w0003amz.toml index ad31ea49a8..b17def5770 100644 --- a/tests/metal/lenovo-20w0003amz.toml +++ b/tests/metal/lenovo-20w0003amz.toml @@ -67,6 +67,9 @@ bios = "N34ET71W (1.71 )" "boot.testcases-window.complete_ms" = 1165 "boot.testcases-window.panel_max_us" = 3822 "boot.testcases-window.panel_us" = 22472 +"boot.usbbreak.complete_ms" = 1160 +"boot.usbbreak.panel_max_us" = 3905 +"boot.usbbreak.panel_us" = 21851 "boot.usbload.complete_ms" = 1165 "boot.usbload.panel_max_us" = 3837 "boot.usbload.panel_us" = 21428