From 0a3a1c85f87185e460e3040dfe5aa681febfd2c5 Mon Sep 17 00:00:00 2001 From: japabu Date: Sun, 27 Sep 2026 21:56:06 +0200 Subject: [PATCH 01/11] xHCI: a torn-down port is read again without waiting for an event A port torn down with its device still in it (a replug collapsed inside the debounce, or a disk refused while another is held for its device) was left `Settled` with `ports_dirty` false. PORTSC's CSC stays set because the teardown step returns ahead of the acknowledge, and QEMU's `xhci_port_notify` raises no event while that bit is set, so nothing looked at the port again until an unrelated event arrived. `PortState::torn_down` now leaves the port `Unread`, which `PortState::outstanding` reports, so `XhciController::poll` steps it on the same pass. Both `AfterSlot::Teardown` and `AfterSlot::Again` reach it through that one function; the second was never staged. `xhci_flap` now paces each cycle's edges on the previous device's bind instead of a fixed 600 ms, so a lost wake is a cycle that never binds and the gate names it, rather than a collapse the next cycle rescues by parity. Its row leaves the disabled list and its issue is closed. Co-Authored-By: Claude Opus 5.5 --- ...ed-only-when-another-port-event-arrives.md | 4 +- src/redlist.rs | 4 - tests/common/usb.rs | 93 ++++++++++++------- toyos-xhci/src/port.rs | 30 +++++- 4 files changed, 86 insertions(+), 45 deletions(-) diff --git a/issues/hardware/a-collapsed-replug-is-enumerated-only-when-another-port-event-arrives.md b/issues/hardware/a-collapsed-replug-is-enumerated-only-when-another-port-event-arrives.md index 4fbcd4a15d..d6c45742a4 100644 --- a/issues/hardware/a-collapsed-replug-is-enumerated-only-when-another-port-event-arrives.md +++ b/issues/hardware/a-collapsed-replug-is-enumerated-only-when-another-port-event-arrives.md @@ -1,5 +1,5 @@ --- -status: expected-red +status: closed kind: defect opened: 2026-09-27 --- @@ -72,3 +72,5 @@ that every collapsed teardown is enumerated before the next cycle's edges is another. `xhci_flap` is disabled in `src/redlist.rs` until then, and the change that meets this deletes its row. Owner: the xHCI driver's port stepping (`kernel/src/drivers/xhci/mod.rs`); held by the orchestrator. + +Closed: `PortState::torn_down` (`toyos-xhci/src/port.rs`) leaves the port `Unread`, which `PortState::outstanding` reports, so the next `XhciController::poll` reads it without waiting for an event. diff --git a/src/redlist.rs b/src/redlist.rs index f941c3eacc..76c3125b29 100644 --- a/src/redlist.rs +++ b/src/redlist.rs @@ -71,10 +71,6 @@ pub const DISABLED: &[Disabled] = &[ test: "usb_disk_index_stable", issue: "issues/hardware/usb-disk-index-stable-nothing-enumerates-on-the-first-controller.md", }, - Disabled { - test: "xhci_flap", - issue: "issues/hardware/a-collapsed-replug-is-enumerated-only-when-another-port-event-arrives.md", - }, ]; /// The row of `rows` that disables `test`, matched by the whole name. diff --git a/tests/common/usb.rs b/tests/common/usb.rs index 333a93fd16..29011c0191 100644 --- a/tests/common/usb.rs +++ b/tests/common/usb.rs @@ -3911,6 +3911,11 @@ pub fn xhci_flap( /// ordinary events, and never the state under test. Measured: the first /// shape of this gate walked ports 5, 6, 7, 8 and staged nothing. const PORT: &str = "1"; + /// A pointer's bind, which is what each cycle's edges wait for: a port + /// torn down and never looked at again is then a cycle that never binds, + /// since no later cycle's event goes in to wake it. + const BOUND: &str = "xHCI: pointer on slot "; + const COLLAPSED: &str = "was unplugged and plugged back in between two looks"; let options = BootOptions { profile: Profile::MetalHotplug, @@ -3924,46 +3929,44 @@ pub fn xhci_flap( return Err(format!("the kernel never said what pointer scale it used:\n{boot}")); }; - let result = qemu.run_test_hooked( - "test_rs_input_events", - Duration::from_secs(60), - "===INPUT_READY===", - move |socket| { - let mut devices = qemu::QmpDevices::open(socket); - devices.add("usb-mouse", "xhci1.0", "flap0", &[("port", PORT)]); - drop(devices); - thread::sleep(Duration::from_millis(600)); - - for cycle in 0..CYCLES { - let mut devices = qemu::QmpDevices::open(socket); - // No sleep between the two: both edges have to land inside one + // Each move waits for the guest to print the one before it. + let (mut ready, mut binds, mut mev) = (false, 0usize, 0usize); + let mut input: Option = None; + let result = qemu.run_test_paced("test_rs_input_events", Duration::from_secs(60), |socket, line| { + let qmp = || socket.expect("xhci_flap needs BootOptions { qmp: true }"); + if line.contains("===INPUT_READY===") { + ready = true; + qemu::QmpDevices::open(qmp()).add("usb-mouse", "xhci1.0", "flap0", &[("port", PORT)]); + return; + } + if ready && line.contains(BOUND) { + binds += 1; + if binds <= CYCLES { + let cycle = binds - 1; + let mut devices = qemu::QmpDevices::open(qmp()); + // No wait between the two: both edges have to land inside one // 100 ms debounce, which is the whole point. A fresh id each // cycle because `device_del` releases the old one // asynchronously and a reused one races with that. devices.del(&format!("flap{cycle}")); - devices.add( - "usb-mouse", - "xhci1.0", - &format!("flap{}", cycle + 1), - &[("port", PORT)], - ); - drop(devices); - // Long enough for the driver to finish acting on the cycle - // before the next one starts, so what the log shows is - // CYCLES collapsed replugs and not one long blur. - thread::sleep(Duration::from_millis(600)); + devices.add("usb-mouse", "xhci1.0", &format!("flap{}", cycle + 1), &[("port", PORT)]); + } else if binds == CYCLES + 1 { + // The pointer that is in the port now has to work. Off the + // origin first: the accumulated position clamps at 0. + input.insert(qemu::QmpInput::open(qmp())).mouse(100, 100, None); } - - // The pointer that is in the port now has to work. Off the origin - // first: the accumulated position clamps at 0. - let mut input = qemu::QmpInput::open(socket); - input.mouse(100, 100, None); - thread::sleep(Duration::from_millis(100)); - input.mouse(DX, DY, None); - thread::sleep(Duration::from_millis(200)); - crate::input_events_end(&mut input); - }, - ); + return; + } + let Some(input) = input.as_mut() else { return }; + if line.contains("mev buttons=") { + mev += 1; + match mev { + 1 => input.mouse(DX, DY, None), + 2 => crate::input_events_end(input), + _ => {} + } + } + }); if let Some(err) = &result.error { return Err(format!("{err}\n{}\n{}", result.serial, result.stdout)); } @@ -3974,10 +3977,28 @@ pub fn xhci_flap( } } + // **Every cycle's device bound before the next cycle's edges went in**, so + // a cycle that never bound is named by the last thing its port did. + if binds != CYCLES + 1 { + let last = log.lines().filter(|l| l.contains("xHCI: port ") || l.contains(BOUND)).last(); + let why = match last { + _ if binds > CYCLES + 1 => "more binds than plugs", + Some(line) if line.contains(COLLAPSED) => { + "a collapsed replug was torn down and its port never looked at again" + } + _ => "the device in the port never bound", + }; + return Err(format!( + "{why}: {binds} bind(s) for {} plugs before the guest's input window closed; the \ + port's last line was {last:?}\n{log}", + CYCLES + 1 + )); + } + // The race was actually staged. Without this the gate would pass on a run // where every replug happened to be seen as two distinct states, which is // the easy case and not the one under test. - let collapsed = log.matches("was unplugged and plugged back in between two looks").count(); + let collapsed = log.matches(COLLAPSED).count(); if collapsed == 0 { // The two ways this fires read alike and are not alike, so the counts // that tell them apart are in the message. A driver that saw every diff --git a/toyos-xhci/src/port.rs b/toyos-xhci/src/port.rs index 8adf21015a..f174d597ac 100644 --- a/toyos-xhci/src/port.rs +++ b/toyos-xhci/src/port.rs @@ -263,6 +263,10 @@ enum Work { Resetting { until: Nanos, kind: Reset }, /// The caller is inside an effect and has not reported it. Working(Effect), + /// A teardown moved the driver's belief and nothing has read the register + /// against it since. Outstanding, so the next pass looks: a device left in + /// the port has already spent the change event that would have said so. + Unread, } /// A deliberate defect, compiled only for the negative gates. @@ -365,12 +369,12 @@ impl PortState { } /// The teardown finished. The port is empty as far as the driver is - /// concerned, whatever the register says, so the next look runs the - /// ordinary fresh-connect path. + /// concerned, whatever the register says, and [`Self::outstanding`] until + /// the next look, which runs the ordinary fresh-connect path. pub fn torn_down(&mut self) { self.attached = false; self.slot = None; - self.work = Work::Settled; + self.work = Work::Unread; } /// Adopt a port the boot scan enumerated, so the hot-plug machine starts @@ -494,8 +498,9 @@ impl PortState { } let held = match self.work { - Work::Settled => { + Work::Settled | Work::Unread => { if connected == self.attached { + self.work = Work::Settled; return Step::Idle; } self.work = Work::Debouncing { at: now }; @@ -586,6 +591,23 @@ mod tests { assert_eq!(inherited(None, trained), Reset::Hot); } + /// A device still in a torn-down port raises no further change event, so + /// the teardown itself is what has the port read again. + #[test] + fn a_torn_down_port_is_outstanding_until_it_is_read() { + let mut port = PortState::EMPTY; + port.adopt(NonZeroU8::new(1)); + assert!(!port.outstanding()); + port.torn_down(); + assert!(port.outstanding(), "nothing would read a port the device is still in"); + assert!(matches!(port.step(connected(true, 0), 0), Step::Wait(DEBOUNCE_NS))); + + port.torn_down(); + assert!(port.outstanding()); + assert!(matches!(port.step(Portsc::from_raw(1 << 9), 0), Step::Idle)); + assert!(!port.outstanding(), "an empty port read once is at rest"); + } + #[test] fn a_device_its_class_reset_did_not_bring_back_gets_the_most_reset_its_port_has() { assert_eq!(offline_reset(Some(Protocol::Usb3)), Reset::Warm); From e7ad9350bbbf92f2ca0283766eca61341b120090 Mon Sep 17 00:00:00 2001 From: japabu Date: Sun, 27 Sep 2026 22:11:07 +0200 Subject: [PATCH 02/11] issues: a sysroot cloned while stage2 had no cargo stays broken Found while running xhci_flap's negative control: sysroot 5dc157f7fac727be was cloned from stage2 at 22:03 and stage2's cargo link was made at 22:08, so the sysroot has no cargo and every build against its key panics in assert_toolchain_is_honest. Filed, not fixed: it is off this branch's path. Co-Authored-By: Claude Opus 5.5 --- ...-while-stage2-had-no-cargo-stays-broken.md | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 issues/build/a-sysroot-cloned-while-stage2-had-no-cargo-stays-broken.md diff --git a/issues/build/a-sysroot-cloned-while-stage2-had-no-cargo-stays-broken.md b/issues/build/a-sysroot-cloned-while-stage2-had-no-cargo-stays-broken.md new file mode 100644 index 0000000000..c7fce329c8 --- /dev/null +++ b/issues/build/a-sysroot-cloned-while-stage2-had-no-cargo-stays-broken.md @@ -0,0 +1,42 @@ +--- +status: open +kind: tooling +opened: 2026-09-27 +--- + +# A sysroot cloned while stage2 had no cargo stays broken for its key + +`sysroot::build` (`src/sysroot.rs`) makes a sysroot by `clone_tree` of the +primary's `stage2`, and only `provision_toolchain_cargo` (`src/toolchain.rs`) +puts `bin/cargo` there. A sysroot cloned in the window between a compiler +rebuild and that provisioning has no `cargo`; `finished()` reads only +`SOURCES`, so nothing rebuilds it, and every build against its key panics in +`assert_toolchain_is_honest`. + +## Evidence + +On the dev host, primary at `c5518949`: + +- `rust/build/aarch64-apple-darwin/stage2/` modified 22:00:14, + `stage2/bin/rustdoc` 21:55, `stage2/bin/cargo` (the link to + `nightly-aarch64-apple-darwin/bin/cargo`) made 22:08:04. +- `rust/build/sysroots/5dc157f7fac727be/` and its `SOURCES` 22:03:59, built by + pid 20399 (`[build-lock] ... held by pid 20399 (building sysroot + 5dc157f7fac727be)`), `SOURCES` naming `fork + /Users/jan/Dev/jan/toyos-nokthread/rust`. Its `bin/` holds `rustc` and + `rustdoc` and no `cargo`. +- `cargo test --test toyos-build -- --nightly xhci_flap` in + `/Users/jan/Dev/jan/toyos-xhciwake` then exits 101 before any guest boots: + every C corpus entry reports `the toyos toolchain at + .../sysroots/5dc157f7fac727be/bin is missing cargo` (log finished 22:04:13). + The same command in the same worktree exited 0 in a run whose log finished + 21:55:26. + +Which step left `stage2` without its link between 22:00 and 22:08 is not +measured. + +## Exit condition + +A sysroot is never marked finished without the `cargo` its toolchain needs, or +one found without it is rebuilt or provisioned where `ensure` finds it. Owner: +`src/sysroot.rs` and `src/toolchain.rs`; held by the orchestrator. From 22db7531312470a75759e99c4a119020ff706f24 Mon Sep 17 00:00:00 2001 From: japabu Date: Sun, 27 Sep 2026 22:40:28 +0200 Subject: [PATCH 03/11] issues: a collapsed replug is fixed, so its issue goes; the cargo-less sysroot is the toolchain fix's The tracker closes an issue by deleting its file (issues/README.md). Co-Authored-By: Claude Opus 5.5 --- ...-while-stage2-had-no-cargo-stays-broken.md | 42 ---------- ...ed-only-when-another-port-event-arrives.md | 76 ------------------- 2 files changed, 118 deletions(-) delete mode 100644 issues/build/a-sysroot-cloned-while-stage2-had-no-cargo-stays-broken.md delete mode 100644 issues/hardware/a-collapsed-replug-is-enumerated-only-when-another-port-event-arrives.md diff --git a/issues/build/a-sysroot-cloned-while-stage2-had-no-cargo-stays-broken.md b/issues/build/a-sysroot-cloned-while-stage2-had-no-cargo-stays-broken.md deleted file mode 100644 index c7fce329c8..0000000000 --- a/issues/build/a-sysroot-cloned-while-stage2-had-no-cargo-stays-broken.md +++ /dev/null @@ -1,42 +0,0 @@ ---- -status: open -kind: tooling -opened: 2026-09-27 ---- - -# A sysroot cloned while stage2 had no cargo stays broken for its key - -`sysroot::build` (`src/sysroot.rs`) makes a sysroot by `clone_tree` of the -primary's `stage2`, and only `provision_toolchain_cargo` (`src/toolchain.rs`) -puts `bin/cargo` there. A sysroot cloned in the window between a compiler -rebuild and that provisioning has no `cargo`; `finished()` reads only -`SOURCES`, so nothing rebuilds it, and every build against its key panics in -`assert_toolchain_is_honest`. - -## Evidence - -On the dev host, primary at `c5518949`: - -- `rust/build/aarch64-apple-darwin/stage2/` modified 22:00:14, - `stage2/bin/rustdoc` 21:55, `stage2/bin/cargo` (the link to - `nightly-aarch64-apple-darwin/bin/cargo`) made 22:08:04. -- `rust/build/sysroots/5dc157f7fac727be/` and its `SOURCES` 22:03:59, built by - pid 20399 (`[build-lock] ... held by pid 20399 (building sysroot - 5dc157f7fac727be)`), `SOURCES` naming `fork - /Users/jan/Dev/jan/toyos-nokthread/rust`. Its `bin/` holds `rustc` and - `rustdoc` and no `cargo`. -- `cargo test --test toyos-build -- --nightly xhci_flap` in - `/Users/jan/Dev/jan/toyos-xhciwake` then exits 101 before any guest boots: - every C corpus entry reports `the toyos toolchain at - .../sysroots/5dc157f7fac727be/bin is missing cargo` (log finished 22:04:13). - The same command in the same worktree exited 0 in a run whose log finished - 21:55:26. - -Which step left `stage2` without its link between 22:00 and 22:08 is not -measured. - -## Exit condition - -A sysroot is never marked finished without the `cargo` its toolchain needs, or -one found without it is rebuilt or provisioned where `ensure` finds it. Owner: -`src/sysroot.rs` and `src/toolchain.rs`; held by the orchestrator. diff --git a/issues/hardware/a-collapsed-replug-is-enumerated-only-when-another-port-event-arrives.md b/issues/hardware/a-collapsed-replug-is-enumerated-only-when-another-port-event-arrives.md deleted file mode 100644 index d6c45742a4..0000000000 --- a/issues/hardware/a-collapsed-replug-is-enumerated-only-when-another-port-event-arrives.md +++ /dev/null @@ -1,76 +0,0 @@ ---- -status: closed -kind: defect -opened: 2026-09-27 ---- - -# A collapsed replug is enumerated only when another port event arrives - -A device pulled and pushed back between two looks is torn down -(`Step::Teardown(Gone::Replugged)`), and its slot's Disable Slot completes in a -later `Controller::poll` (`kernel/src/drivers/xhci/mod.rs`). There -`slot_gone`'s `AfterSlot::Teardown` calls `PortState::torn_down`, which leaves -the port `Settled` and not attached with the device still in it. `poll` steps -the ports only when `ports_dirty` is set or a port is `outstanding`. Neither is -true then, so nothing looks at the port again. `PORT_WORK_AT` goes to 0, and -PORTSC's CSC stays set because the teardown step returns ahead of the -acknowledge. QEMU raises no further Port Status Change for that port -(`xhci_port_notify` returns while the bit is set), so the device in the port is -never enumerated. It stays dead until an unrelated event marks the ports dirty. -The next unplug is one such event. On the T14 this is a replugged mouse that -does not come back. - -Whether the wake is lost depends on how QEMU's two edge events fall across -polls: `xhci_port_update` clears PORTSC and then notifies, so the detach and -the attach each raise their own event. The wake survives when the attach's -event is drained in the same poll as the completion or later. It is lost when -both events are drained before the completion. - -## Evidence - -- The PR #535 nightly (run 36314576406, `guest (1)`, a4f68c5a) was red with - `0 slot(s) enabled and never disabled ([]) after 4 replugs`. The first three - collapses re-enumerated 100 ms after their teardown (1.780 → 1.881 s). The - fourth was torn down at 3.586 s, and nothing followed in the 900 ms before - the guest's input ended. `wt/toyos-lld` at a55d62c6, an ancestor of `main` - (run 36287592139, `guest (1)`), was red with the same sentence. Its first - collapse (1.971 s) was enumerated only at 2.674 s, 700 ms later, when the - next cycle's edges arrived. -- The dev host, QEMU 11.1.1, TCG, on `nightly-green2` at 877b8c95. QEMU's - `hw/usb/hcd-xhci.c` is byte-identical at v11.1.0 and v11.1.1. With only a - print of the serial added, every other collapse sits about 700 ms until the - next cycle rescues it: 1.022 → 1.725 s and 2.232 → 2.938 s. The test is - green because the fourth cycle is a rescue. With `self.ports_dirty = true;` - added after `torn_down()` in `AfterSlot::Teardown`, all four collapses are - seen as such and each re-enumerates 100 ms after its teardown - (`4 replugs collapsed inside the debounce (4 seen as such): 5 slot(s) - enabled`). -- The same tree with `CYCLES = 3`: red, `EXIT=1`, `0 slot(s) enabled and never - disabled ([]) after 3 replugs`, red again in the harness's alone re-run. - With the one-line wake added it is green, `EXIT=0`, `3 seen as such`. -- One named run of `xhci_flap` as committed is green on `nightly-green2` - (`EXIT=0`) and on `main` at 16d2e645 (`EXIT=0`). -- PR #542's nightly at 059c5de7 (run 36328646395, `guest (2)`), whose diff - touches no driver or guest code, was red with the same sentence. The - collapse torn down at 1.523 s was enumerated at 2.225 s, when the next - cycle's edges arrived; the one torn down at 3.328 s had nothing after it - before the guest's input ended at 4.234 s. - -So the gate as committed passes by parity wherever every collapse loses its -wake. On the dev host it cannot go red. It reds on CI's KVM shards only when -the last collapse is the one that loses it. - -`AfterSlot::Again` ends in the same `torn_down()` with no look after it; that -arm was not staged. - -## Exit condition - -A port torn down with its device still in it is looked at again without -waiting for another event, shown by a gate that goes red on the lost wake on -every host. `xhci_flap` at an odd cycle count is one such gate. An assertion -that every collapsed teardown is enumerated before the next cycle's edges is -another. `xhci_flap` is disabled in `src/redlist.rs` until then, and the change -that meets this deletes its row. Owner: the xHCI driver's port stepping -(`kernel/src/drivers/xhci/mod.rs`); held by the orchestrator. - -Closed: `PortState::torn_down` (`toyos-xhci/src/port.rs`) leaves the port `Unread`, which `PortState::outstanding` reports, so the next `XhciController::poll` reads it without waiting for an event. From 06aeca8ae7d58a14e7d93d1ef75d7d2151fde08a Mon Sep 17 00:00:00 2001 From: japabu Date: Sun, 27 Sep 2026 23:02:06 +0200 Subject: [PATCH 04/11] xhci_flap: cut the checks five binds made unreachable; file two xHCI issues xhci_flap already requires CYCLES + 1 binds, and it counts the same "xHCI: pointer on slot ... merges as source" line that the sources check reads. So the empty-sources check can no longer fire. A run with `collapsed == 0` can only be one whose every replug was seen as distinct, so the dead-port reading goes and the message keeps its first sentence. Binds now pace on `parse_pointer_sources`, not on a second copy of its literal. Filed: - issues/kernel/a-disk-refused-while-one-is-held-is-enumerated-again-and-no-test-reaches-that-arm.md - issues/kernel/a-port-given-up-on-or-enumerated-after-its-change-event-is-spent-is-never-read-again.md The second issue is the review's simulator NOTE, measured. A pure `port::due` that both loops call, plus a hub that raises an event only on a change bit's 0->1 edge, turns `repeated_replugs_stay_balanced` red on c5518949's port.rs with 1 teardown for 4 replugs. It also turns two more sim tests red on this branch, one for a GaveUp and one for an enumeration's end, and three more production lines in port.rs are needed to make them green. That is beyond the brief's one function, so the patch is not landed. Co-Authored-By: Claude Opus 5.5 --- ...ated-again-and-no-test-reaches-that-arm.md | 25 ++++++++ ...ange-event-is-spent-is-never-read-again.md | 57 +++++++++++++++++++ tests/common/usb.rs | 27 ++------- 3 files changed, 86 insertions(+), 23 deletions(-) create mode 100644 issues/kernel/a-disk-refused-while-one-is-held-is-enumerated-again-and-no-test-reaches-that-arm.md create mode 100644 issues/kernel/a-port-given-up-on-or-enumerated-after-its-change-event-is-spent-is-never-read-again.md diff --git a/issues/kernel/a-disk-refused-while-one-is-held-is-enumerated-again-and-no-test-reaches-that-arm.md b/issues/kernel/a-disk-refused-while-one-is-held-is-enumerated-again-and-no-test-reaches-that-arm.md new file mode 100644 index 0000000000..a115d929cf --- /dev/null +++ b/issues/kernel/a-disk-refused-while-one-is-held-is-enumerated-again-and-no-test-reaches-that-arm.md @@ -0,0 +1,25 @@ +--- +status: open +kind: defect +opened: 2026-09-27 +--- + +# A disk refused while one is held is enumerated again, and no test reaches that arm + +`refuse_for_now` (`kernel/src/drivers/xhci/device.rs`) gives a disk refused as +not ready, or for want of a pool block, a Disable Slot with +`AfterSlot::Again` while a disk on this controller is held for its device. The +arm in `slot_gone` (`kernel/src/drivers/xhci/mod.rs`) frees the refused +device's block, tears its port down so it is enumerated again, and logs +`xHCI: port N is enumerated again while a disk is held for its device`. + +Nothing reads that line: `rg 'enumerated again while a disk' tests/` finds +nothing, and no run has shown whether any guest test reaches the arm. The +decision is taken in the kernel and not in `toyos-xhci`, so no host test can +stage it either. + +**Owner**: the xHCI driver, `kernel/src/drivers/xhci/`. + +**Exit**: a test stages a disk refused while another is held, and asserts that +its port is enumerated again and the disk bound. With `AfterSlot::Again` +replaced by `AfterSlot::Refused`, that test goes red. diff --git a/issues/kernel/a-port-given-up-on-or-enumerated-after-its-change-event-is-spent-is-never-read-again.md b/issues/kernel/a-port-given-up-on-or-enumerated-after-its-change-event-is-spent-is-never-read-again.md new file mode 100644 index 0000000000..089fb83c73 --- /dev/null +++ b/issues/kernel/a-port-given-up-on-or-enumerated-after-its-change-event-is-spent-is-never-read-again.md @@ -0,0 +1,57 @@ +--- +status: open +kind: defect +opened: 2026-09-27 +--- + +# A port given up on, or enumerated, after its change event is spent is never read again + +The kernel steps its ports only when a Port Status Change Event arrived, or +when a port has work of its own (`Controller::poll`, +`kernel/src/drivers/xhci/mod.rs`). xHCI raises that event only on a change +bit's 0→1 edge, as QEMU's `xhci_port_notify` does. So a port that ends its work +in `Work::Settled` without reading the register again has spent its edge, and it +is looked at again only when the next edge arrives. Two paths end that way: + +- **`GaveUp`.** A USB3 device pulled during the warm retrain that follows a + failed bus reset: the retrain finds nothing, the port is given up on as + attached, and `service_port` returns. The device's absence is never torn down. +- **The end of an enumeration.** A device pulled while Enable Slot is + outstanding. Enable Slot is never cancelled, so when it ends silent `finish` + reports the port attached and acknowledges its change flags, and nothing + reads it again. + +Both self-heal on the next plug's connect edge. + +**The host simulator cannot show this.** `toyos-xhci/sim/src/driver.rs`'s +`pump` steps the port on every pass, and its hub raises no event at all. No +host test can fail on the gating inside `poll` either: moving the `outstanding` +test above `advance_outstanding` keeps every host suite green. + +**Measured** with a patch that is not landed. It adds a pure +`port::due(signalled, ports)` that both loops call, a hub that raises an event +only on a change bit's 0→1 edge, and a pump that steps only where `due` says so. +Run with `cargo test -p toyos-xhci -p toyos-xhci-sim --all-features`: + +- `superspeed::a_device_pulled_during_the_warm_retrain_is_not_enumerated` goes + red with `[Reset(Hot), Reset(Warm), GaveUp(LinkNeverTrained)]` and no + teardown. +- `enumerate::gate_an_enumeration_that_outlives_its_port_costs_a_deadline` goes + red with "the port was never freed". +- A probe that pulls the device while Enable Slot is silent is green with + per-pass stepping, and red with the edge rule, still attached with + `[Reset(Hot), Enumerated { slot: None, trained: false }]`. +- `scenarios::repeated_replugs_stay_balanced` goes red on `port.rs` as of + c5518949, with 1 teardown for 4 replugs. It is green once `torn_down` leaves + the port `Work::Unread`. +- Three more lines in `toyos-xhci/src/port.rs` make every suite and the probe + green: `enumerated` and both `GaveUp` transitions leave the port + `Work::Unread`, not `Work::Settled`. + +**Owner**: the xHCI driver, `kernel/src/drivers/xhci/` and `toyos-xhci/`. + +**Exit**: `Controller::poll` and the simulator decide whether to step through +one function in `toyos-xhci`. The simulated hub raises an event only on a change +bit's 0→1 edge. No port reaches `Work::Settled` except from a read that found it +as the driver believes. The two tests above are green, and +`repeated_replugs_stay_balanced` is red on c5518949's `port.rs`. diff --git a/tests/common/usb.rs b/tests/common/usb.rs index 29011c0191..31ce93473e 100644 --- a/tests/common/usb.rs +++ b/tests/common/usb.rs @@ -3911,10 +3911,6 @@ pub fn xhci_flap( /// ordinary events, and never the state under test. Measured: the first /// shape of this gate walked ports 5, 6, 7, 8 and staged nothing. const PORT: &str = "1"; - /// A pointer's bind, which is what each cycle's edges wait for: a port - /// torn down and never looked at again is then a cycle that never binds, - /// since no later cycle's event goes in to wake it. - const BOUND: &str = "xHCI: pointer on slot "; const COLLAPSED: &str = "was unplugged and plugged back in between two looks"; let options = BootOptions { @@ -3928,6 +3924,7 @@ pub fn xhci_flap( let Some((scale_x, scale_y)) = crate::parse_rel_scale(&boot) else { return Err(format!("the kernel never said what pointer scale it used:\n{boot}")); }; + let bound = |line: &str| !crate::parse_pointer_sources(line).is_empty(); // Each move waits for the guest to print the one before it. let (mut ready, mut binds, mut mev) = (false, 0usize, 0usize); @@ -3939,7 +3936,7 @@ pub fn xhci_flap( qemu::QmpDevices::open(qmp()).add("usb-mouse", "xhci1.0", "flap0", &[("port", PORT)]); return; } - if ready && line.contains(BOUND) { + if ready && bound(line) { binds += 1; if binds <= CYCLES { let cycle = binds - 1; @@ -3980,7 +3977,7 @@ pub fn xhci_flap( // **Every cycle's device bound before the next cycle's edges went in**, so // a cycle that never bound is named by the last thing its port did. if binds != CYCLES + 1 { - let last = log.lines().filter(|l| l.contains("xHCI: port ") || l.contains(BOUND)).last(); + let last = log.lines().filter(|l| l.contains("xHCI: port ") || bound(l)).last(); let why = match last { _ if binds > CYCLES + 1 => "more binds than plugs", Some(line) if line.contains(COLLAPSED) => { @@ -4000,21 +3997,8 @@ pub fn xhci_flap( // the easy case and not the one under test. let collapsed = log.matches(COLLAPSED).count(); if collapsed == 0 { - // The two ways this fires read alike and are not alike, so the counts - // that tell them apart are in the message. A driver that saw every - // cycle as a distinct disconnect enumerated once per cycle; one that - // could not see a collapsed replug at all enumerated **once**, left the - // slot bound to the device that had gone, and delivered nothing — which - // is what the pre-fix driver does here, and a good deal worse than the - // slot march the same defect produces when the replugs are slow enough - // to be seen. return Err(format!( - "no replug collapsed inside a debounce, so this run never staged the race. The guest \ - bound {} pointer(s) across {CYCLES} cycles and delivered {} pointer event(s): one \ - bind and no events is a dead port, one bind per cycle is a run whose replugs were \ - all seen as distinct.\n{log}", - crate::parse_pointer_sources(log).len(), - crate::parse_mouse_events(&result.stdout).len(), + "no replug collapsed inside a debounce, so this run never staged the race.\n{log}" )); } @@ -4049,9 +4033,6 @@ pub fn xhci_flap( // **Sources reclaimed.** One pointer is in the port at a time, so every // bind must print the same button-table entry. A leak marches 2, 3, 4, 5. let sources: Vec = crate::parse_pointer_sources(log).iter().map(|(_, s)| *s).collect(); - if sources.is_empty() { - return Err(format!("no pointer bound during the flap at all\n{log}")); - } if sources.iter().any(|s| *s != sources[0]) { return Err(format!( "pointer sources were {sources:?} — a replugged pointer took a fresh button-table \ From f254ed3da517241276d92e594639fb6cdca3d5e3 Mon Sep 17 00:00:00 2001 From: japabu Date: Sun, 27 Sep 2026 23:04:24 +0200 Subject: [PATCH 05/11] xhci_flap: name a stalled cycle with rfind, which clippy's warnings-denied gate wants `filter(..).last()` on a double-ended iterator is `clippy::double_ended_iterator_last`, and the host lane denies warnings. Co-Authored-By: Claude Opus 5.5 --- tests/common/usb.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/common/usb.rs b/tests/common/usb.rs index 31ce93473e..66ab856850 100644 --- a/tests/common/usb.rs +++ b/tests/common/usb.rs @@ -3977,7 +3977,7 @@ pub fn xhci_flap( // **Every cycle's device bound before the next cycle's edges went in**, so // a cycle that never bound is named by the last thing its port did. if binds != CYCLES + 1 { - let last = log.lines().filter(|l| l.contains("xHCI: port ") || bound(l)).last(); + let last = log.lines().rfind(|l| l.contains("xHCI: port ") || bound(l)); let why = match last { _ if binds > CYCLES + 1 => "more binds than plugs", Some(line) if line.contains(COLLAPSED) => { From da0be3993a4763e901f51184dd5164c6cc3a27a6 Mon Sep 17 00:00:00 2001 From: japabu Date: Sun, 27 Sep 2026 23:22:59 +0200 Subject: [PATCH 06/11] xHCI: every report that moves a port's belief leaves it to be read xHCI raises a Port Status Change Event only on a change bit's 0->1 edge, so a port whose device changed while the driver was inside an effect has spent its edge. `PortState::believe` is now the only setter of `attached` and `slot`, and it leaves the port `Work::Unread`: `torn_down`, `enumerated`, `adopt` and both `GaveUp` transitions go through it. - `port::due(signalled, ports)` is the one decision whether a pass steps the ports; `Controller::poll` and the simulator's pump both call it. - The simulated hub changes its register through one setter that raises the event on a change flag's 0->1 edge, and the pump steps the port only where `due` says so. With it, the simulator finds what per-pass stepping hid: `enumerated` and the two `GaveUp` transitions left the port Settled with its edge spent. - The GaveUp arm of `service_port` (and of the pump) reads the port again in the same pass instead of returning: nothing else wakes a pass for a port left to be read. The pump now fails `Stuck::Unwoken` when a pass leaves the port outstanding with no instant to come back at. - The boot scan's last store to PORT_WORK_AT asks for a pass when a bound port is outstanding, since the scan's acknowledge spent its edge. - New sim test: a device pulled while Enable Slot goes unanswered is torn down once the deadline ends the enumeration. Host arms, each red: port.rs as of c5518949 with `due` and the edge rule kept (repeated_replugs_stay_balanced, a_replug_inside_one_debounce_is_seen); the three transitions and the GaveUp return reverted (a_device_pulled_during_the_warm_retrain_is_not_enumerated, gate_an_enumeration_that_outlives_its_port_costs_a_deadline, the new Enable Slot test); the GaveUp return alone (Unwoken, seven tests); `adopt` back to Settled (the port unit test). The issue this fixes is deleted; the acknowledge after an enumeration, which clears a replug no look has seen, is filed. Co-Authored-By: Claude Opus 5.5 --- ...ange-event-is-spent-is-never-read-again.md | 57 ------------- ...ration-clears-a-replug-no-look-has-seen.md | 33 ++++++++ kernel/src/drivers/xhci/mod.rs | 4 +- kernel/src/drivers/xhci/wait/boot.rs | 6 +- toyos-xhci/sim/src/driver.rs | 26 ++++-- toyos-xhci/sim/src/hub.rs | 47 +++++++---- toyos-xhci/sim/tests/enumerate.rs | 21 +++++ toyos-xhci/src/port.rs | 84 ++++++++++++------- 8 files changed, 164 insertions(+), 114 deletions(-) delete mode 100644 issues/kernel/a-port-given-up-on-or-enumerated-after-its-change-event-is-spent-is-never-read-again.md create mode 100644 issues/kernel/an-acknowledge-after-an-enumeration-clears-a-replug-no-look-has-seen.md diff --git a/issues/kernel/a-port-given-up-on-or-enumerated-after-its-change-event-is-spent-is-never-read-again.md b/issues/kernel/a-port-given-up-on-or-enumerated-after-its-change-event-is-spent-is-never-read-again.md deleted file mode 100644 index 089fb83c73..0000000000 --- a/issues/kernel/a-port-given-up-on-or-enumerated-after-its-change-event-is-spent-is-never-read-again.md +++ /dev/null @@ -1,57 +0,0 @@ ---- -status: open -kind: defect -opened: 2026-09-27 ---- - -# A port given up on, or enumerated, after its change event is spent is never read again - -The kernel steps its ports only when a Port Status Change Event arrived, or -when a port has work of its own (`Controller::poll`, -`kernel/src/drivers/xhci/mod.rs`). xHCI raises that event only on a change -bit's 0→1 edge, as QEMU's `xhci_port_notify` does. So a port that ends its work -in `Work::Settled` without reading the register again has spent its edge, and it -is looked at again only when the next edge arrives. Two paths end that way: - -- **`GaveUp`.** A USB3 device pulled during the warm retrain that follows a - failed bus reset: the retrain finds nothing, the port is given up on as - attached, and `service_port` returns. The device's absence is never torn down. -- **The end of an enumeration.** A device pulled while Enable Slot is - outstanding. Enable Slot is never cancelled, so when it ends silent `finish` - reports the port attached and acknowledges its change flags, and nothing - reads it again. - -Both self-heal on the next plug's connect edge. - -**The host simulator cannot show this.** `toyos-xhci/sim/src/driver.rs`'s -`pump` steps the port on every pass, and its hub raises no event at all. No -host test can fail on the gating inside `poll` either: moving the `outstanding` -test above `advance_outstanding` keeps every host suite green. - -**Measured** with a patch that is not landed. It adds a pure -`port::due(signalled, ports)` that both loops call, a hub that raises an event -only on a change bit's 0→1 edge, and a pump that steps only where `due` says so. -Run with `cargo test -p toyos-xhci -p toyos-xhci-sim --all-features`: - -- `superspeed::a_device_pulled_during_the_warm_retrain_is_not_enumerated` goes - red with `[Reset(Hot), Reset(Warm), GaveUp(LinkNeverTrained)]` and no - teardown. -- `enumerate::gate_an_enumeration_that_outlives_its_port_costs_a_deadline` goes - red with "the port was never freed". -- A probe that pulls the device while Enable Slot is silent is green with - per-pass stepping, and red with the edge rule, still attached with - `[Reset(Hot), Enumerated { slot: None, trained: false }]`. -- `scenarios::repeated_replugs_stay_balanced` goes red on `port.rs` as of - c5518949, with 1 teardown for 4 replugs. It is green once `torn_down` leaves - the port `Work::Unread`. -- Three more lines in `toyos-xhci/src/port.rs` make every suite and the probe - green: `enumerated` and both `GaveUp` transitions leave the port - `Work::Unread`, not `Work::Settled`. - -**Owner**: the xHCI driver, `kernel/src/drivers/xhci/` and `toyos-xhci/`. - -**Exit**: `Controller::poll` and the simulator decide whether to step through -one function in `toyos-xhci`. The simulated hub raises an event only on a change -bit's 0→1 edge. No port reaches `Work::Settled` except from a read that found it -as the driver believes. The two tests above are green, and -`repeated_replugs_stay_balanced` is red on c5518949's `port.rs`. diff --git a/issues/kernel/an-acknowledge-after-an-enumeration-clears-a-replug-no-look-has-seen.md b/issues/kernel/an-acknowledge-after-an-enumeration-clears-a-replug-no-look-has-seen.md new file mode 100644 index 0000000000..44556c82b0 --- /dev/null +++ b/issues/kernel/an-acknowledge-after-an-enumeration-clears-a-replug-no-look-has-seen.md @@ -0,0 +1,33 @@ +--- +status: open +kind: defect +opened: 2026-09-27 +--- + +# An acknowledge after an enumeration clears a replug no look has seen + +`device::finish`, `device::refuse` and `device::refuse_for_now` +(`kernel/src/drivers/xhci/device.rs`) report the port with +`PortState::enumerated` and then call `acknowledge_port_read`, which writes +back every change flag the read found, CSC included. `wait::boot::scan_ports` +does the same for every port once the boot scan is over +(`acknowledge_port_changes`). + +A replug that lands after the port's last look and before that acknowledge is +cleared before any step reads it. One way in: its Port Status Change Event is +drained by the same `poll` whose `advance_outstanding` ends the enumeration, so +`service_ports` runs after `finish` has written CSC back. The port then reads +connected with no CSC, which is what the driver believes, and the device now in +it is never torn down or enumerated. The look `PortState::believe` owes the +port catches a pull, because CCS reads 0, and not a replug. + +**Not measured.** No test stages it. The simulator's `enumerated` +(`toyos-xhci/sim/src/driver.rs`) acknowledges nothing, so no host test can see +it either. + +**Owner**: the xHCI driver, `kernel/src/drivers/xhci/`. + +**Exit**: the simulator's report acknowledges exactly what the kernel's does, +and a sim test that replugs the device between an enumeration's last answer +and the next look expects `ToreDown(Replugged)` and a second enumeration. That +test is red on the kernel's acknowledge and green without it. diff --git a/kernel/src/drivers/xhci/mod.rs b/kernel/src/drivers/xhci/mod.rs index f5c2efb8be..049498a051 100644 --- a/kernel/src/drivers/xhci/mod.rs +++ b/kernel/src/drivers/xhci/mod.rs @@ -1271,7 +1271,7 @@ impl XhciController { portsc.raw() ), } - return None; + // No return: the port is left to be read, and nothing else wakes a pass for it. } Step::Write(write) => self.write_portsc(port_idx, write), Step::Reset(kind, write) => { @@ -1522,7 +1522,7 @@ impl XhciController { // Nothing below reads the event ring: every step `service_ports` takes is a submit, so one advance is enough. let mut wake_at = None; - if self.ports_dirty || self.ports.iter().any(PortState::outstanding) { + if portmachine::due(self.ports_dirty, &self.ports) { self.ports_dirty = false; wake_at = self.service_ports(); } diff --git a/kernel/src/drivers/xhci/wait/boot.rs b/kernel/src/drivers/xhci/wait/boot.rs index e815cff200..8116add67d 100644 --- a/kernel/src/drivers/xhci/wait/boot.rs +++ b/kernel/src/drivers/xhci/wait/boot.rs @@ -140,8 +140,10 @@ pub fn init(devices: &[PciDevice]) { } return; } - // Safe to zero: the boot scan acted on every port it looked at, so nothing is outstanding. - PORT_WORK_AT.store(0, Ordering::Relaxed); + // A port the scan bound is outstanding until the first pass reads it: the scan's acknowledge spent its edge. + let due = controllers.iter().any(|c| port::due(false, &c.ports)); + let at = if due { crate::clock::nanos_since_boot().max(1) } else { 0 }; + PORT_WORK_AT.store(at, Ordering::Relaxed); let hid: usize = controllers.iter().map(|c| c.devices.len()).sum(); log!("xHCI: {} controller(s), {} HID device(s)", controllers.len(), hid); log!("usb-storage: {} device(s)", storage_count()); diff --git a/toyos-xhci/sim/src/driver.rs b/toyos-xhci/sim/src/driver.rs index 3261ff32b3..2f26b96b40 100644 --- a/toyos-xhci/sim/src/driver.rs +++ b/toyos-xhci/sim/src/driver.rs @@ -1,8 +1,9 @@ //! The loop the kernel runs, with the effects replaced by a record of them. //! //! Deliberately the *shape* the kernel takes and not a convenience: drain the -//! controller's answers, act on whatever is finished, read the register, ask the -//! machine, do the one thing it says, read again. A simulator whose loop differs +//! controller's answers, act on whatever is finished, and where +//! [`port::due`] says so read the register, ask the machine, do the one thing +//! it says, read again. A simulator whose loop differs //! from the driver's tests a driver nobody ships. //! //! **Nothing here can wait**, and that is the property under test as much as any @@ -43,6 +44,9 @@ pub enum Stuck { /// a future instant. A live-lock is a failure whatever it looks like from /// inside. NoProgress, + /// A pass left the port with work of its own and no instant to come back + /// at, so the kernel would not step it again until some other event. + Unwoken, } /// What the loop must never do, whatever sequence produced it. @@ -310,10 +314,22 @@ impl Driver { /// Everything the driver has to do at `now`, with every step checked /// against the word that produced it. pub fn pump(&mut self, port: &mut FakePort, now: Nanos) -> Result<(), Stuck> { + self.pass(port, now)?; + if self.state.outstanding() && self.wake_at.is_none() { + return Err(Stuck::Unwoken); + } + Ok(()) + } + + fn pass(&mut self, port: &mut FakePort, now: Nanos) -> Result<(), Stuck> { port.tick(now); self.collect(now); self.advance(now)?; self.recover(port, now); + if !port::due(port.take_event(), core::slice::from_ref(&self.state)) { + self.wake_at = self.outstanding.wake_at(); + return Ok(()); + } for _ in 0..STEP_BUDGET { let read = port.read(); if !read.connected() || read.connect_changed() { @@ -341,11 +357,7 @@ impl Driver { self.wake_at = Some(at); return Ok(()); } - Step::GaveUp(why) => { - self.did.push(Did::GaveUp(why)); - self.wake_at = self.outstanding.wake_at(); - return Ok(()); - } + Step::GaveUp(why) => self.did.push(Did::GaveUp(why)), Step::Write(write) => port.write(write.raw(), now), Step::Reset(kind, write) => { self.did.push(Did::Reset(kind)); diff --git a/toyos-xhci/sim/src/hub.rs b/toyos-xhci/sim/src/hub.rs index 9d538e65f3..c5b6c884eb 100644 --- a/toyos-xhci/sim/src/hub.rs +++ b/toyos-xhci/sim/src/hub.rs @@ -6,7 +6,9 @@ //! write-1-to-clear, PR is write-1-to-set and the *controller* clears it, PED //! is set by a reset that finds a device and cleared by a write of '1'. The //! last of those is what QEMU does not implement and what disabled every port -//! on the laptop. +//! on the laptop. A Port Status Change Event is raised only where a change +//! flag goes from 0 to 1, so a driver that looks only when told misses what +//! no edge reports. use toyos_xhci::port::Nanos; use toyos_xhci::Portsc; @@ -64,6 +66,8 @@ pub struct FakePort { /// Every write the driver made, for the assertions that are about what it /// did rather than about where it ended up. pub writes: Vec, + /// A change flag went from 0 to 1 since the driver last took the event. + signalled: bool, } impl FakePort { @@ -77,6 +81,7 @@ impl FakePort { present: false, superspeed: false, writes: Vec::new(), + signalled: false, } } @@ -97,6 +102,12 @@ impl FakePort { self.raw } + /// Whether a Port Status Change Event has been raised since the last call, + /// taking it. + pub fn take_event(&mut self) -> bool { + core::mem::take(&mut self.signalled) + } + /// A SuperSpeed port. Its link trains itself: a device appearing brings the /// port to Enabled with the link at U0 and no reset from anybody, which is /// §4.19.1.2's own sequence and the thing a USB2-shaped driver resets away. @@ -109,9 +120,9 @@ impl FakePort { return; } self.present = true; - self.raw |= CCS | CSC; + self.set(self.raw | CCS | CSC); if self.superspeed { - self.raw |= PED | ((self.speed as u32) << SPEED_SHIFT); + self.set(self.raw | PED | ((self.speed as u32) << SPEED_SHIFT)); self.set_link(PLS_U0); } } @@ -121,7 +132,7 @@ impl FakePort { pub fn detach(&mut self) { self.present = false; if self.raw & CCS != 0 { - self.raw = (self.raw & !(CCS | PED | PR)) | CSC; + self.set((self.raw & !(CCS | PED | PR)) | CSC); self.resetting_since = None; } } @@ -157,7 +168,7 @@ impl FakePort { next |= self.raw & PR; } let started = next & PR != 0 && self.raw & PR == 0; - self.raw = next; + self.set(next); if started { self.warm = warm; self.resetting_since = Some(now); @@ -176,7 +187,7 @@ impl FakePort { // The link went down when the hot reset hit it, and it is // not coming back on its own. self.set_link(PLS_INACTIVE); - self.raw &= !(PR | PED); + self.set(self.raw & !(PR | PED)); self.resetting_since = None; return; } @@ -189,11 +200,11 @@ impl FakePort { if !self.warm { // §4.19.5's completed failure, bit for bit. self.resetting_since = None; - self.raw &= !(PR | PED | (0xF << SPEED_SHIFT)); + self.set(self.raw & !(PR | PED | (0xF << SPEED_SHIFT))); if self.raw & CCS != 0 { - self.raw = (self.raw & !CCS) | CSC; + self.set((self.raw & !CCS) | CSC); } - self.raw |= PRC; + self.set(self.raw | PRC); self.set_link(PLS_RX_DETECT); return; } @@ -208,23 +219,29 @@ impl FakePort { } self.resetting_since = None; let warm = self.warm; - self.raw &= !(PR | WPR); - self.raw |= PRC; + self.set(self.raw & !(PR | WPR)); + self.set(self.raw | PRC); if warm { - self.raw |= WRC; + self.set(self.raw | WRC); // §4.19.5's failure lost *detection* only; the retrain finds // whatever is physically there, connect edge and all. if self.present && self.raw & CCS == 0 { - self.raw |= CCS | CSC; + self.set(self.raw | CCS | CSC); } } if self.raw & CCS != 0 { - self.raw |= PED | ((self.speed as u32) << SPEED_SHIFT); + self.set(self.raw | PED | ((self.speed as u32) << SPEED_SHIFT)); self.set_link(PLS_U0); } } fn set_link(&mut self, pls: u32) { - self.raw = (self.raw & !(0xF << PLS_SHIFT)) | (pls << PLS_SHIFT); + self.set((self.raw & !(0xF << PLS_SHIFT)) | (pls << PLS_SHIFT)); + } + + /// The one way the register changes, so no edge goes unraised. + fn set(&mut self, next: u32) { + self.signalled |= next & CHANGES & !self.raw != 0; + self.raw = next; } } diff --git a/toyos-xhci/sim/tests/enumerate.rs b/toyos-xhci/sim/tests/enumerate.rs index b3283dbd87..c64310ef64 100644 --- a/toyos-xhci/sim/tests/enumerate.rs +++ b/toyos-xhci/sim/tests/enumerate.rs @@ -201,3 +201,24 @@ fn a_slot_the_controller_has_not_named_yet_is_waited_for_even_when_the_port_goes assert_eq!(driver.abandoned, 0, "the Enable Slot was abandoned and its slot with it"); assert!(driver.busy(), "nothing is listening for the slot the controller was asked for"); } + +/// The same Enable Slot, waited out. Its unplug's edge was spent while the port +/// was inside the enumeration, so the report that ends it is what has the port +/// read again, and the device that left is taken down a debounce later. +#[test] +fn a_device_pulled_while_enable_slot_goes_unanswered_is_torn_down() { + let mut port = FakePort::occupied(QUICK); + let mut driver = Driver::new().answering(Answers::Never); + let began = until_busy(&mut driver, &mut port, SETTLED); + port.detach(); + let end = began + ANSWER_DEADLINE_NS + 2 * DEBOUNCE_NS; + driver.run_to(&mut port, began, end, PASS).unwrap(); + + assert!( + driver.did.contains(&Did::Enumerated { slot: None, trained: false }), + "the Enable Slot's deadline never ended the enumeration: {:?}", + driver.did + ); + assert_eq!(driver.teardowns(), 1, "the device that left was never taken down: {:?}", driver.did); + assert!(!driver.attached(), "{:?}", driver.did); +} diff --git a/toyos-xhci/src/port.rs b/toyos-xhci/src/port.rs index f174d597ac..9e96d80323 100644 --- a/toyos-xhci/src/port.rs +++ b/toyos-xhci/src/port.rs @@ -121,6 +121,16 @@ pub fn enumeration_ack(after: Option, portsc: Portsc) -> portsc::Write { } } +/// Whether a pass steps the ports, given whether a Port Status Change Event or +/// a caller's own reason to look has arrived since the last one. +/// +/// A port with work of its own is stepped without an event: xHCI raises one +/// only on a change bit's 0→1 edge, and a port whose belief has just moved may +/// hold a device whose edge is already spent ([`PortState::believe`]). +pub fn due(signalled: bool, ports: &[PortState]) -> bool { + signalled || ports.iter().any(PortState::outstanding) +} + /// Why a port stopped being worked on. #[derive(Clone, Copy, PartialEq, Eq, Debug)] pub enum GaveUp { @@ -263,9 +273,8 @@ enum Work { Resetting { until: Nanos, kind: Reset }, /// The caller is inside an effect and has not reported it. Working(Effect), - /// A teardown moved the driver's belief and nothing has read the register - /// against it since. Outstanding, so the next pass looks: a device left in - /// the port has already spent the change event that would have said so. + /// The driver's belief moved ([`PortState::believe`]) and nothing has read + /// the register against it since. Unread, } @@ -365,20 +374,16 @@ impl PortState { /// not. Recorded either way: an Enable Slot that succeeded is the /// controller's resource whatever happened after it. pub fn enumerated(&mut self, slot: Option) { - self.adopt(slot); + self.believe(true, slot); } /// The teardown finished. The port is empty as far as the driver is - /// concerned, whatever the register says, and [`Self::outstanding`] until - /// the next look, which runs the ordinary fresh-connect path. + /// concerned, whatever the register says, so the next look runs the + /// ordinary fresh-connect path. pub fn torn_down(&mut self) { - self.attached = false; - self.slot = None; - self.work = Work::Unread; + self.believe(false, None); } - /// Adopt a port the boot scan enumerated, so the hot-plug machine starts - /// from what the boot path already did rather than re-deciding it. /// What the controller's Supported Protocol capability said this port /// speaks. Set once, at bring-up, from firmware's own description of the /// machine. @@ -386,10 +391,22 @@ impl PortState { self.protocol = protocol; } + /// Adopt a port the boot scan enumerated, so the hot-plug machine starts + /// from what the boot path already did rather than re-deciding it. pub fn adopt(&mut self, slot: Option) { - self.attached = true; + self.believe(true, slot); + } + + /// **The one place the driver's belief about a port is set, and it leaves + /// the port [`Self::outstanding`] until a look has read the register + /// against it.** Every caller has just ended an effect or given up on one, + /// and the device in the port may have changed meanwhile with no change + /// bit going 0→1 again — the only edge xHCI raises an event for — because + /// an effect's own acknowledge spent it. + fn believe(&mut self, attached: bool, slot: Option) { + self.attached = attached; self.slot = slot; - self.work = Work::Settled; + self.work = Work::Unread; } #[cfg(feature = "flaws")] @@ -442,8 +459,7 @@ impl PortState { return Step::Reset(Reset::Warm, write); } ResetOutcome::GaveUp(why) => { - self.attached = true; - self.work = Work::Settled; + self.believe(true, self.slot); return Step::GaveUp(why); } } @@ -462,8 +478,7 @@ impl PortState { // Attached, so the port is not tried again until its device is // pulled — which is what stops a port the controller will not reset // from being reset forever. - self.attached = true; - self.work = Work::Settled; + self.believe(true, self.slot); return Step::GaveUp(match kind { Reset::Warm => GaveUp::LinkNeverTrained, Reset::Hot => GaveUp::ResetNeverFinished(Reset::Hot), @@ -591,21 +606,28 @@ mod tests { assert_eq!(inherited(None, trained), Reset::Hot); } - /// A device still in a torn-down port raises no further change event, so - /// the teardown itself is what has the port read again. + /// Every report that moves the belief leaves the port to be read: a device + /// that changed under the effect raises no further change event. #[test] - fn a_torn_down_port_is_outstanding_until_it_is_read() { - let mut port = PortState::EMPTY; - port.adopt(NonZeroU8::new(1)); - assert!(!port.outstanding()); - port.torn_down(); - assert!(port.outstanding(), "nothing would read a port the device is still in"); - assert!(matches!(port.step(connected(true, 0), 0), Step::Wait(DEBOUNCE_NS))); - - port.torn_down(); - assert!(port.outstanding()); - assert!(matches!(port.step(Portsc::from_raw(1 << 9), 0), Step::Idle)); - assert!(!port.outstanding(), "an empty port read once is at rest"); + fn a_port_whose_belief_moved_is_outstanding_until_it_is_read() { + let empty = Portsc::from_raw(1 << 9); + let reports = [ + ("adopt", (|p| p.adopt(NonZeroU8::new(1))) as fn(&mut PortState)), + ("enumerated", |p| p.enumerated(NonZeroU8::new(1))), + ("torn_down", PortState::torn_down), + ]; + for (name, report) in reports { + let mut port = PortState::EMPTY; + report(&mut port); + assert!(port.outstanding(), "{name}: nothing would read a port whose device changed"); + let (disagrees, agrees) = + if port.attached() { (empty, connected(true, 0)) } else { (connected(true, 0), empty) }; + assert!(matches!(port.step(disagrees, 0), Step::Wait(DEBOUNCE_NS)), "{name}"); + + report(&mut port); + assert!(matches!(port.step(agrees, 0), Step::Idle), "{name}"); + assert!(!port.outstanding(), "{name}: a port read once and found as believed is at rest"); + } } #[test] From 304197858f297b6efb8c2c5ce035c17b5d1967ae Mon Sep 17 00:00:00 2001 From: japabu Date: Mon, 28 Sep 2026 00:11:52 +0200 Subject: [PATCH 07/11] xHCI: a port whose enumeration begin refuses is read again in the same pass `device::begin` can refuse before Enable Slot: the port reads disabled after the enumeration's acknowledge, or it came up at a speed with no packet size or no Protocol Speed ID. `finish` then reports the port attached with no slot and leaves it `Unread`, and `service_port` returned `outstanding.wake_at()`, which is `None` when nothing else is outstanding. The port was left to be read with no pass scheduled. This is reachable on a USB3 port. A device is pulled between the step that saw its warm completion and `begin`'s read. The retrain's CSC is still set, so the pull raises no event. `enumeration_ack(Some(Warm))` clears CSC, and `begin` finds PED clear. `service_port`'s `Enumerate` arm no longer returns. The loop reads the port again: a begun enumeration is caught at the loop's top as working and returns its wake as before; a refused one is stepped. The simulator's `Enumerate` arm now mirrors `begin`'s `!enabled` refusal, which it did not model, and `Driver::pulled_as_it_enumerates` stages the pull at the site where the reentrancy check already runs. `a_device_pulled_as_its_enumeration_begins_is_torn_down` expects `[Reset(Hot), Reset(Warm), Enumerated { slot: None, trained: false }, ToreDown(Disconnected)]` with no command spent. It goes red with `Unwoken` when the refusal returns the way the kernel did, and with `Enumerated { slot: Some(1) }` when the refusal is not modelled. The boot scan's store to `PORT_WORK_AT` is deleted, and so is the store of 0 it replaced. It is not load-bearing. The ISR records every interrupt the scan's own resets and commands raise, and the first `poll_if_pending` on that CPU steps every port the scan left `Unread`. The only scan that raises none binds no port except those given up on with no slot, because a reset that never finished raises no event. There, the next connect's edge is read as a replug. Whatever `PORT_WORK_AT` holds at that point is harmless: a nonzero value costs one poll, and that poll stores the right value. `believe`'s doc loses "the one place the driver's belief about a port is set", since `take_slot` also sets `slot`. Filed: - issues/kernel/a-reset-given-up-on-with-its-connect-flag-set-is-torn-down-and-retried-every-debounce.md - issues/kernel/a-poll-that-leaves-a-port-outstanding-with-no-wake-says-nothing.md Co-Authored-By: Claude Opus 5.5 --- ...t-outstanding-with-no-wake-says-nothing.md | 20 +++++++++++++ ...is-torn-down-and-retried-every-debounce.md | 28 +++++++++++++++++++ kernel/src/drivers/xhci/mod.rs | 3 +- kernel/src/drivers/xhci/wait/boot.rs | 7 +---- toyos-xhci/sim/src/driver.rs | 20 +++++++++++++ toyos-xhci/sim/tests/superspeed.rs | 27 ++++++++++++++++++ toyos-xhci/src/port.rs | 5 ++-- 7 files changed, 99 insertions(+), 11 deletions(-) create mode 100644 issues/kernel/a-poll-that-leaves-a-port-outstanding-with-no-wake-says-nothing.md create mode 100644 issues/kernel/a-reset-given-up-on-with-its-connect-flag-set-is-torn-down-and-retried-every-debounce.md diff --git a/issues/kernel/a-poll-that-leaves-a-port-outstanding-with-no-wake-says-nothing.md b/issues/kernel/a-poll-that-leaves-a-port-outstanding-with-no-wake-says-nothing.md new file mode 100644 index 0000000000..c8e2419afa --- /dev/null +++ b/issues/kernel/a-poll-that-leaves-a-port-outstanding-with-no-wake-says-nothing.md @@ -0,0 +1,20 @@ +--- +status: open +kind: defect +opened: 2026-09-28 +--- + +# A poll that leaves a port outstanding with no wake says nothing + +The simulator's pump (`toyos-xhci/sim/src/driver.rs`) fails with +`Stuck::Unwoken` when a pass leaves the port outstanding +(`PortState::outstanding`) with no instant to come back at. The kernel has no +counterpart. `XhciController::poll` (`kernel/src/drivers/xhci/mod.rs`) returns +`None` in that state, `poll_if_pending` stores 0 in `PORT_WORK_AT`, and the port +is not read again until some other xHCI interrupt arrives. On hardware nothing +names the state, and the simulator catches it only on the arms it models. + +**Owner**: the xHCI driver, `kernel/src/drivers/xhci/`. + +**Exit**: `poll` logs a line naming the port whenever it returns `None` with a +port outstanding. diff --git a/issues/kernel/a-reset-given-up-on-with-its-connect-flag-set-is-torn-down-and-retried-every-debounce.md b/issues/kernel/a-reset-given-up-on-with-its-connect-flag-set-is-torn-down-and-retried-every-debounce.md new file mode 100644 index 0000000000..4d0ce9fc7d --- /dev/null +++ b/issues/kernel/a-reset-given-up-on-with-its-connect-flag-set-is-torn-down-and-retried-every-debounce.md @@ -0,0 +1,28 @@ +--- +status: open +kind: defect +opened: 2026-09-28 +--- + +# A reset given up on with its connect flag set is torn down and retried every debounce + +`PortState::step` (`toyos-xhci/src/port.rs`) gives up on a port whose reset +completed disabled with no escalation left (`GaveUp::LinkNeverTrained`, +`GaveUp::ResetFailed`) and leaves it attached, so that it is not reset again +until its device is pulled. `service_port` (`kernel/src/drivers/xhci/mod.rs`) +and the simulator's pump read the port again in the same pass. + +If the completion left CCS and CSC set (§4.19.5.1: a warm completion carries +the retrain's connect edge), that read is a replug. The port is torn down, +debounced and reset again, and gives up again: once per debounce for as long +as the device stays in, which is the loop `GaveUp` exists to stop. + +**Not measured.** No `ResetBehaviour` in `toyos-xhci/sim/src/hub.rs` completes +a reset with the port connected and disabled, and QEMU completes every reset +enabled. + +**Owner**: the xHCI port machine, `toyos-xhci/src/port.rs`. + +**Exit**: a `ResetBehaviour` whose warm reset completes with CCS and CSC set and +PED clear, and a sim test that holds such a device in its port for many +debounces and expects exactly one `GaveUp` and no teardown. diff --git a/kernel/src/drivers/xhci/mod.rs b/kernel/src/drivers/xhci/mod.rs index 049498a051..7392a8b82a 100644 --- a/kernel/src/drivers/xhci/mod.rs +++ b/kernel/src/drivers/xhci/mod.rs @@ -1316,8 +1316,7 @@ impl XhciController { log!("xHCI: port {} connected, link already trained", port_idx + 1); } device::begin(self, port_idx, after); - // Either enumeration is under way and the port waits, or it refused before spending a command. - return self.outstanding.wake_at(); + // No return: a `begin` that refused before Enable Slot left the port to be read, and a begun one is caught above as working. } } } diff --git a/kernel/src/drivers/xhci/wait/boot.rs b/kernel/src/drivers/xhci/wait/boot.rs index 8116add67d..6dd90063ee 100644 --- a/kernel/src/drivers/xhci/wait/boot.rs +++ b/kernel/src/drivers/xhci/wait/boot.rs @@ -2,7 +2,6 @@ //! happens before there is a scheduler; every wait here runs in place. use alloc::vec::Vec; -use core::sync::atomic::Ordering; use crate::log; use crate::time::{Budget, Cadence, Duration}; @@ -21,7 +20,7 @@ use super::super::{IR0_ERDP, IR0_ERSTBA, IR0_ERSTSZ, IR0_IMAN, IR0_IMOD}; use super::super::{OFF_CMD_RING, OFF_DCBAA, OFF_ERST, OFF_EVT_RING}; use super::super::{OP_CONFIG, OP_CRCR, OP_DCBAAP, OP_PAGESIZE, OP_PORT_BASE, OP_USBCMD, OP_USBSTS}; use super::super::{USBCMD_HCRST, USBCMD_RS, USBSTS_CNR, USBSTS_HCH}; -use super::super::{PORTSC_PP, PORT_REG_SIZE, PORT_WORK_AT, XHCI}; +use super::super::{PORTSC_PP, PORT_REG_SIZE, XHCI}; use super::super::{controller_answers, PORT_DEBOUNCE_NS}; use super::settles; use toyos_xhci::port::{self, GaveUp, Reset, ResetOutcome}; @@ -140,10 +139,6 @@ pub fn init(devices: &[PciDevice]) { } return; } - // A port the scan bound is outstanding until the first pass reads it: the scan's acknowledge spent its edge. - let due = controllers.iter().any(|c| port::due(false, &c.ports)); - let at = if due { crate::clock::nanos_since_boot().max(1) } else { 0 }; - PORT_WORK_AT.store(at, Ordering::Relaxed); let hid: usize = controllers.iter().map(|c| c.devices.len()).sum(); log!("xHCI: {} controller(s), {} HID device(s)", controllers.len(), hid); log!("usb-storage: {} device(s)", storage_count()); diff --git a/toyos-xhci/sim/src/driver.rs b/toyos-xhci/sim/src/driver.rs index 2f26b96b40..c8965bc9c3 100644 --- a/toyos-xhci/sim/src/driver.rs +++ b/toyos-xhci/sim/src/driver.rs @@ -125,6 +125,9 @@ pub struct Driver { /// an enumeration would do. The enumeration drains the event ring, so this /// is reachable rather than hypothetical. reenter: bool, + /// Pull the device between the step that says enumerate and the + /// enumeration's first read of the port. + pull_as_it_enumerates: bool, /// Enumerate and tear down without asking whether the controller still owes /// an answer, which is the negative gate for the deferral. never_defers: bool, @@ -182,6 +185,7 @@ impl Driver { slot: Some(1), spent: None, reenter: false, + pull_as_it_enumerates: false, never_defers: false, never_cancels: false, function: enumerate::Function::BootHid, @@ -240,6 +244,13 @@ impl Driver { self } + /// Stage a device pulled between the step that says enumerate and the + /// enumeration's first read of the port. + pub fn pulled_as_it_enumerates(mut self) -> Self { + self.pull_as_it_enumerates = true; + self + } + /// Stage a device whose configuration descriptor names this function, which /// is the one branch an enumeration's order depends on. pub fn presenting(mut self, function: enumerate::Function) -> Self { @@ -402,6 +413,9 @@ impl Driver { return Err(Stuck::Broke(bad)); } } + if core::mem::take(&mut self.pull_as_it_enumerates) { + port.detach(); + } if self.outstanding.busy() { return Err(Stuck::Order(Broke::ActedWithAnAnswerOutstanding)); } @@ -417,6 +431,12 @@ impl Driver { return Err(Stuck::Broke(bad)); } port.write(ack.raw(), now); + // `device::begin`'s refusal of a port its acknowledge left + // disabled: no command is spent, and the port is read again. + if !port.read().enabled() { + self.enumerated(after.is_none()); + continue; + } // Submitted and left, exactly as the teardown is: the port // stays inside the effect until the last act is answered, // and the check above catches a step taken meanwhile. diff --git a/toyos-xhci/sim/tests/superspeed.rs b/toyos-xhci/sim/tests/superspeed.rs index 16eb850e00..24dc1aca5e 100644 --- a/toyos-xhci/sim/tests/superspeed.rs +++ b/toyos-xhci/sim/tests/superspeed.rs @@ -217,6 +217,33 @@ fn a_device_pulled_during_the_warm_retrain_is_not_enumerated() { assert!(!driver.attached(), "the port is attached with nothing in it: {:?}", driver.did); } +/// The same retrain, with the device **pulled after the step that saw it +/// complete and before the enumeration's first read**. The retrain's connect +/// flag is still set, so the pull raises no event; the enumeration's +/// acknowledge clears it and finds the port disabled. The refusal is what has +/// the port read again. +#[test] +fn a_device_pulled_as_its_enumeration_begins_is_torn_down() { + let mut port = FakePort::occupied(ResetBehaviour::FailsTheBusReset { warm_works: true }); + let mut driver = Driver::new().speaking(Protocol::Usb3).pulled_as_it_enumerates(); + driver + .run_to(&mut port, 0, 4 * DEBOUNCE_NS + RESET_DEADLINE_NS, PASS) + .unwrap(); + + assert_eq!( + driver.did, + [ + Did::Reset(Reset::Hot), + Did::Reset(Reset::Warm), + Did::Enumerated { slot: None, trained: false }, + Did::ToreDown(Gone::Disconnected), + ], + "the pull was not refused before Enable Slot and then taken down" + ); + assert!(driver.acts.is_empty(), "a command was spent on a port that read disabled: {:?}", driver.acts); + assert!(!driver.attached(), "the port is attached with nothing in it: {:?}", driver.did); +} + /// The same failure on a link that will not come back warm either: refused by /// the name the warm-reset dead end already has. #[test] diff --git a/toyos-xhci/src/port.rs b/toyos-xhci/src/port.rs index 9e96d80323..2bc985f2ed 100644 --- a/toyos-xhci/src/port.rs +++ b/toyos-xhci/src/port.rs @@ -397,9 +397,8 @@ impl PortState { self.believe(true, slot); } - /// **The one place the driver's belief about a port is set, and it leaves - /// the port [`Self::outstanding`] until a look has read the register - /// against it.** Every caller has just ended an effect or given up on one, + /// **Leaves the port [`Self::outstanding`] until a look has read the + /// register against it.** Every caller has just ended an effect or given up on one, /// and the device in the port may have changed meanwhile with no change /// bit going 0→1 again — the only edge xHCI raises an event for — because /// an effect's own acknowledge spent it. From f2ba12fbda494ef16c8a8bb1ba818d61490b27b0 Mon Sep 17 00:00:00 2001 From: japabu Date: Mon, 28 Sep 2026 00:34:18 +0200 Subject: [PATCH 08/11] xHCI: a port given up on waits for a fresh edge, and is still read once A reset given up on left the port Unread, and the same pass read it again. A completion that leaves CCS and CSC set -- a warm retrain that re-detects the device and cannot enable it -- then read as a replug: the port was torn down, debounced and reset again, once per debounce for as long as its device stayed in. `PortState::give_up` is now the one rule for both give-ups. The port is attached and `Work::GivenUp`: still outstanding, so it is read once and a pull is seen, but the change flags that read finds are the given-up reset's own and are acknowledged without being judged a replug. Only an edge after that acknowledge moves the port. The simulated hub stages the case as `ResetBehaviour::RetrainsDisabled`, and `a_port_given_up_on_is_not_reset_again_until_its_device_is_pulled` holds such a device for twenty debounces, expects one hot and one warm reset and one GaveUp, then pulls it and expects the teardown. Also: `PortState::adopt` and `port_bound` go, since the boot scan only ever recorded `None`; it calls `enumerated(None)`. The publish of `XHCI` states the invariant the boot store's deletion rests on. The sim's duplicate field doc goes. The issue this fixes is deleted. Co-Authored-By: Claude Opus 5.5 --- ...is-torn-down-and-retried-every-debounce.md | 28 ------------- kernel/src/drivers/xhci/mod.rs | 5 --- kernel/src/drivers/xhci/wait/boot.rs | 5 ++- toyos-xhci/sim/src/driver.rs | 2 - toyos-xhci/sim/src/hub.rs | 36 +++++++++-------- toyos-xhci/sim/tests/superspeed.rs | 30 ++++++++++++++ toyos-xhci/src/port.rs | 40 +++++++++---------- 7 files changed, 73 insertions(+), 73 deletions(-) delete mode 100644 issues/kernel/a-reset-given-up-on-with-its-connect-flag-set-is-torn-down-and-retried-every-debounce.md diff --git a/issues/kernel/a-reset-given-up-on-with-its-connect-flag-set-is-torn-down-and-retried-every-debounce.md b/issues/kernel/a-reset-given-up-on-with-its-connect-flag-set-is-torn-down-and-retried-every-debounce.md deleted file mode 100644 index 4d0ce9fc7d..0000000000 --- a/issues/kernel/a-reset-given-up-on-with-its-connect-flag-set-is-torn-down-and-retried-every-debounce.md +++ /dev/null @@ -1,28 +0,0 @@ ---- -status: open -kind: defect -opened: 2026-09-28 ---- - -# A reset given up on with its connect flag set is torn down and retried every debounce - -`PortState::step` (`toyos-xhci/src/port.rs`) gives up on a port whose reset -completed disabled with no escalation left (`GaveUp::LinkNeverTrained`, -`GaveUp::ResetFailed`) and leaves it attached, so that it is not reset again -until its device is pulled. `service_port` (`kernel/src/drivers/xhci/mod.rs`) -and the simulator's pump read the port again in the same pass. - -If the completion left CCS and CSC set (§4.19.5.1: a warm completion carries -the retrain's connect edge), that read is a replug. The port is torn down, -debounced and reset again, and gives up again: once per debounce for as long -as the device stays in, which is the loop `GaveUp` exists to stop. - -**Not measured.** No `ResetBehaviour` in `toyos-xhci/sim/src/hub.rs` completes -a reset with the port connected and disabled, and QEMU completes every reset -enabled. - -**Owner**: the xHCI port machine, `toyos-xhci/src/port.rs`. - -**Exit**: a `ResetBehaviour` whose warm reset completes with CCS and CSC set and -PED clear, and a sim test that holds such a device in its port for many -debounces and expects exactly one `GaveUp` and no teardown. diff --git a/kernel/src/drivers/xhci/mod.rs b/kernel/src/drivers/xhci/mod.rs index 7392a8b82a..36c5b4f5a6 100644 --- a/kernel/src/drivers/xhci/mod.rs +++ b/kernel/src/drivers/xhci/mod.rs @@ -1188,11 +1188,6 @@ impl XhciController { } } - /// Record what the boot scan's enumeration left behind, so hot-plug starts from it; recorded even with no device, since a successful Enable Slot is the controller's resource regardless. - fn port_bound(&mut self, port_idx: u8, slot: Option) { - self.ports[port_idx as usize].adopt(slot.and_then(NonZeroU8::new)); - } - /// Step every port that is not where the driver left it, and say when it wants to be looked at again. /// /// One step per call, no wait; the enumeration it eventually starts is submit-and-return too. diff --git a/kernel/src/drivers/xhci/wait/boot.rs b/kernel/src/drivers/xhci/wait/boot.rs index 6dd90063ee..f4ff31b293 100644 --- a/kernel/src/drivers/xhci/wait/boot.rs +++ b/kernel/src/drivers/xhci/wait/boot.rs @@ -158,6 +158,7 @@ pub fn init(devices: &[PciDevice]) { ctrl.max_ports, ); } + // No scheduler pass runs before `smp::set_ready`, so the scan's interrupt record is first polled with `XHCI` published. *XHCI.lock() = controllers; } @@ -491,7 +492,7 @@ pub fn init_device(ctrl: &mut XhciController, port_idx: u8, protocol: Option { - if !self.warm { - // §4.19.5's completed failure, bit for bit. - self.resetting_since = None; - self.set(self.raw & !(PR | PED | (0xF << SPEED_SHIFT))); - if self.raw & CCS != 0 { - self.set((self.raw & !CCS) | CSC); - } - self.set(self.raw | PRC); - self.set_link(PLS_RX_DETECT); - return; - } - if !warm_works { - return; + ResetBehaviour::FailsTheBusReset { .. } | ResetBehaviour::RetrainsDisabled + if !self.warm => + { + // §4.19.5's completed failure, bit for bit. + self.resetting_since = None; + self.set(self.raw & !(PR | PED | (0xF << SPEED_SHIFT))); + if self.raw & CCS != 0 { + self.set((self.raw & !CCS) | CSC); } - 1_000_000 + self.set(self.raw | PRC); + self.set_link(PLS_RX_DETECT); + return; } + ResetBehaviour::FailsTheBusReset { warm_works: false } => return, + ResetBehaviour::FailsTheBusReset { warm_works: true } + | ResetBehaviour::RetrainsDisabled => 1_000_000, }; if now.saturating_sub(since) < after { return; @@ -229,7 +233,7 @@ impl FakePort { self.set(self.raw | CCS | CSC); } } - if self.raw & CCS != 0 { + if self.raw & CCS != 0 && self.behaviour != ResetBehaviour::RetrainsDisabled { self.set(self.raw | PED | ((self.speed as u32) << SPEED_SHIFT)); self.set_link(PLS_U0); } diff --git a/toyos-xhci/sim/tests/superspeed.rs b/toyos-xhci/sim/tests/superspeed.rs index 24dc1aca5e..c7525e53ce 100644 --- a/toyos-xhci/sim/tests/superspeed.rs +++ b/toyos-xhci/sim/tests/superspeed.rs @@ -261,6 +261,36 @@ fn a_completed_failure_that_stays_failed_is_refused_by_name() { ); } +/// A retrain that completes with the device detected and the port disabled +/// leaves its connect flag set as it gives up. **The port is reset once and +/// not again while its device stays in**, and the pull that ends it is still +/// seen. +#[test] +fn a_port_given_up_on_is_not_reset_again_until_its_device_is_pulled() { + let mut port = FakePort::occupied(ResetBehaviour::RetrainsDisabled); + let mut driver = Driver::new().speaking(Protocol::Usb3); + let held = 2 * RESET_DEADLINE_NS + 20 * DEBOUNCE_NS; + driver.run_to(&mut port, 0, held, PASS).unwrap(); + + assert_eq!( + driver.did, + [Did::Reset(Reset::Hot), Did::Reset(Reset::Warm), Did::GaveUp(GaveUp::LinkNeverTrained)], + "a port given up on was torn down and reset again: {} reset(s) in {held} ns", + driver.resets().len() + ); + assert!(port.read().connected(), "the device left: PORTSC {:#010x}", port.raw()); + + port.detach(); + driver.run_to(&mut port, held, held + 4 * DEBOUNCE_NS, PASS).unwrap(); + assert_eq!( + driver.did.last(), + Some(&Did::ToreDown(Gone::Disconnected)), + "the pull of a port given up on was never seen: {:?}", + driver.did + ); + assert!(!driver.attached(), "{:?}", driver.did); +} + /// "USB2 protocol ports never fail" (§4.19.5): a controller that completes /// one disabled anyway is refused by name, never written a WPR it lacks. #[test] diff --git a/toyos-xhci/src/port.rs b/toyos-xhci/src/port.rs index 2bc985f2ed..e8a8699afc 100644 --- a/toyos-xhci/src/port.rs +++ b/toyos-xhci/src/port.rs @@ -276,6 +276,9 @@ enum Work { /// The driver's belief moved ([`PortState::believe`]) and nothing has read /// the register against it since. Unread, + /// A reset was given up on ([`PortState::give_up`]) and no read has found + /// the port's change flags clear since. + GivenUp, } /// A deliberate defect, compiled only for the negative gates. @@ -391,14 +394,8 @@ impl PortState { self.protocol = protocol; } - /// Adopt a port the boot scan enumerated, so the hot-plug machine starts - /// from what the boot path already did rather than re-deciding it. - pub fn adopt(&mut self, slot: Option) { - self.believe(true, slot); - } - /// **Leaves the port [`Self::outstanding`] until a look has read the - /// register against it.** Every caller has just ended an effect or given up on one, + /// register against it.** Every caller has just ended an effect, /// and the device in the port may have changed meanwhile with no change /// bit going 0→1 again — the only edge xHCI raises an event for — because /// an effect's own acknowledge spent it. @@ -408,6 +405,17 @@ impl PortState { self.work = Work::Unread; } + /// **Attached, so the port is not reset again until a fresh edge moves + /// it**, and read once, so a pull is still seen. The change flags that + /// read finds are the given-up reset's own; a connect flag among them + /// judged as a replug would tear the port down and reset it again every + /// debounce for as long as its device stayed in. + fn give_up(&mut self, why: GaveUp) -> Step<'static> { + self.attached = true; + self.work = Work::GivenUp; + Step::GaveUp(why) + } + #[cfg(feature = "flaws")] pub fn with_flaw(flaw: Flaw) -> Self { Self { flaw, ..Self::EMPTY } @@ -457,10 +465,7 @@ impl PortState { Work::Resetting { until: now + RESET_DEADLINE_NS, kind: Reset::Warm }; return Step::Reset(Reset::Warm, write); } - ResetOutcome::GaveUp(why) => { - self.believe(true, self.slot); - return Step::GaveUp(why); - } + ResetOutcome::GaveUp(why) => return self.give_up(why), } } if now < until || self.flawed(Flaw::NoResetDeadline) { @@ -474,11 +479,7 @@ impl PortState { self.work = Work::Resetting { until: now + RESET_DEADLINE_NS, kind: Reset::Warm }; return Step::Reset(Reset::Warm, reset_write(Reset::Warm, portsc)); } - // Attached, so the port is not tried again until its device is - // pulled — which is what stops a port the controller will not reset - // from being reset forever. - self.believe(true, self.slot); - return Step::GaveUp(match kind { + return self.give_up(match kind { Reset::Warm => GaveUp::LinkNeverTrained, Reset::Hot => GaveUp::ResetNeverFinished(Reset::Hot), }); @@ -495,7 +496,7 @@ impl PortState { // the device that was here is gone. The ordinary teardown then sets // `attached` false, which turns the rest of this into the fresh connect // it already knows how to run. - if connected && replugged && self.attached { + if connected && replugged && self.attached && self.work != Work::GivenUp { return Step::Teardown(Gone::Replugged, Pending(self)); } @@ -512,7 +513,7 @@ impl PortState { } let held = match self.work { - Work::Settled | Work::Unread => { + Work::Settled | Work::Unread | Work::GivenUp => { if connected == self.attached { self.work = Work::Settled; return Step::Idle; @@ -611,8 +612,7 @@ mod tests { fn a_port_whose_belief_moved_is_outstanding_until_it_is_read() { let empty = Portsc::from_raw(1 << 9); let reports = [ - ("adopt", (|p| p.adopt(NonZeroU8::new(1))) as fn(&mut PortState)), - ("enumerated", |p| p.enumerated(NonZeroU8::new(1))), + ("enumerated", (|p| p.enumerated(NonZeroU8::new(1))) as fn(&mut PortState)), ("torn_down", PortState::torn_down), ]; for (name, report) in reports { From caee542b28f9e1292c852dbbd84318d90acd3eaa Mon Sep 17 00:00:00 2001 From: japabu Date: Mon, 28 Sep 2026 01:31:28 +0200 Subject: [PATCH 09/11] xHCI: a hot give-up's connect flag is judged, and the boot scan gives up by the same rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `PortState::gave_up` is the one rule for what a give-up leaves. A warm give-up stays `Work::GivenUp`: §4.19.5.1's retrain raises a connect edge of its own, and nothing tells it apart from a replug. A hot give-up (`ResetNeverFinished(Hot)`, `ResetFailed(Hot)`) goes through `believe`, because a hot reset changes no connect state and a connect flag there is a real replug. Before this, a device replugged inside a hot reset that was then given up on stayed unenumerated until it was pulled again. `a_device_replugged_inside_a_hot_reset_given_up_on_is_enumerated` stages it: at f2ba12fb it records `[Reset(Hot), GaveUp(ResetNeverFinished(Hot))]` and 0 enumerations. The boot scan's two give-ups report through `gave_up` instead of `enumerated(None)`, and `GaveUp::never_finished` is the one mapping from an unfinished reset to its `GaveUp`, which the hot-plug machine uses too. Co-Authored-By: Claude Opus 5.5 --- kernel/src/drivers/xhci/wait/boot.rs | 4 +-- toyos-xhci/sim/tests/scenarios.rs | 16 +++++++++ toyos-xhci/src/port.rs | 53 +++++++++++++++++++++------- 3 files changed, 58 insertions(+), 15 deletions(-) diff --git a/kernel/src/drivers/xhci/wait/boot.rs b/kernel/src/drivers/xhci/wait/boot.rs index f4ff31b293..9eb80beb01 100644 --- a/kernel/src/drivers/xhci/wait/boot.rs +++ b/kernel/src/drivers/xhci/wait/boot.rs @@ -492,7 +492,7 @@ pub fn init_device(ctrl: &mut XhciController, port_idx: u8, protocol: Option Self { + match kind { + Reset::Warm => GaveUp::LinkNeverTrained, + Reset::Hot => GaveUp::ResetNeverFinished(Reset::Hot), + } + } + + /// The reset that was given up on. + fn reset(self) -> Reset { + match self { + GaveUp::ResetNeverFinished(kind) | GaveUp::ResetFailed(kind) => kind, + GaveUp::LinkNeverTrained => Reset::Warm, + } + } +} + /// What a completed reset means for the port — **the one place that question /// is answered**, for [`reset_needed`]'s reason: §4.19.5's failure signature /// (PRC set, the port still disabled) must route the boot scan and the @@ -276,8 +294,8 @@ enum Work { /// The driver's belief moved ([`PortState::believe`]) and nothing has read /// the register against it since. Unread, - /// A reset was given up on ([`PortState::give_up`]) and no read has found - /// the port's change flags clear since. + /// A warm reset was given up on ([`PortState::gave_up`]) and no read has + /// found the port's change flags clear since. GivenUp, } @@ -405,14 +423,26 @@ impl PortState { self.work = Work::Unread; } - /// **Attached, so the port is not reset again until a fresh edge moves - /// it**, and read once, so a pull is still seen. The change flags that - /// read finds are the given-up reset's own; a connect flag among them - /// judged as a replug would tear the port down and reset it again every - /// debounce for as long as its device stayed in. + /// The port was given up on: **attached, so it is not reset again until a + /// fresh edge moves it**, and read once, so a pull is still seen. + pub fn gave_up(&mut self, why: GaveUp) { + match why.reset() { + // §4.19.5.1's retrain raises a connect edge of its own, which + // nothing tells apart from a replug; judged as one, it would tear + // the port down and reset it again every debounce for as long as + // its device stayed in. + Reset::Warm => { + self.attached = true; + self.work = Work::GivenUp; + } + // A hot reset changes no connect state, so a connect flag is a + // real replug. + Reset::Hot => self.believe(true, self.slot), + } + } + fn give_up(&mut self, why: GaveUp) -> Step<'static> { - self.attached = true; - self.work = Work::GivenUp; + self.gave_up(why); Step::GaveUp(why) } @@ -479,10 +509,7 @@ impl PortState { self.work = Work::Resetting { until: now + RESET_DEADLINE_NS, kind: Reset::Warm }; return Step::Reset(Reset::Warm, reset_write(Reset::Warm, portsc)); } - return self.give_up(match kind { - Reset::Warm => GaveUp::LinkNeverTrained, - Reset::Hot => GaveUp::ResetNeverFinished(Reset::Hot), - }); + return self.give_up(GaveUp::never_finished(kind)); } if self.flawed(Flaw::AcknowledgeBeforeDeciding) && portsc.any_change() { From 430b077e00945708a537a2668881b72aea3283ae Mon Sep 17 00:00:00 2001 From: japabu Date: Mon, 28 Sep 2026 02:03:50 +0200 Subject: [PATCH 10/11] issues: usb_transport_break's AnotherStick red is a held disk no CPU took a pass for The `AnotherStick` boot of `usb_transport_break --nightly` at e889d03e went red: port 3 read connected and was never stepped, so the other stick was never refused by name. Nothing on the port machine's path ran. Every call of `PortState::gave_up` is preceded by a log line naming the give-up, in `service_port` and in the boot scan, and the boot has none of them. There is no `port 1 disconnected` line either, though port 1 read empty. - a-held-disk-waits-for-a-pass-no-cpu-takes-when-every-cpu-is-in-a-call-on-it: this boot, where cpu0 spent 4 s in logd's create under `vfs::lock()` and cpu1 took no pass until it ended. - a-shutdown-on-a-held-usb-disk-left-a-cpu-deaf-to-a-tlb-shootdown: the same boot's panic at 9.425 s, with the roles swapped. - New: a device swapped into its port inside the port rung's reset is taken for the disk it replaced. The rung re-reads no identity, and the acknowledge it shares with a warm retrain spends the swap's CSC. Co-Authored-By: Claude Opus 5.5 --- ...reset-is-taken-for-the-disk-it-replaced.md | 36 +++++++++++++++++++ ...takes-when-every-cpu-is-in-a-call-on-it.md | 19 ++++++++++ ...disk-left-a-cpu-deaf-to-a-tlb-shootdown.md | 12 ++++++- 3 files changed, 66 insertions(+), 1 deletion(-) create mode 100644 issues/kernel/a-device-swapped-into-its-port-inside-the-port-rungs-reset-is-taken-for-the-disk-it-replaced.md diff --git a/issues/kernel/a-device-swapped-into-its-port-inside-the-port-rungs-reset-is-taken-for-the-disk-it-replaced.md b/issues/kernel/a-device-swapped-into-its-port-inside-the-port-rungs-reset-is-taken-for-the-disk-it-replaced.md new file mode 100644 index 0000000000..3d76a6a42e --- /dev/null +++ b/issues/kernel/a-device-swapped-into-its-port-inside-the-port-rungs-reset-is-taken-for-the-disk-it-replaced.md @@ -0,0 +1,36 @@ +--- +status: open +kind: defect +opened: 2026-09-28 +--- + +# A device swapped into its port inside the port rung's reset is taken for the disk it replaced + +Derived from the code, not staged. + +The port rung (`port_reset_recovery` in `kernel/src/drivers/xhci/wait/msc.rs`) +resets the port and reads it once (`reset_port`). When +`toyos_xhci::ladder::after_reset` answers `Enumerate`, the port reads +connected, enabled and at the speed the disk was bound at. The rung then +addresses the disk's own slot again, sets its old configuration, adds its old +bulk pair and asks TEST UNIT READY. **Nothing re-reads who the device is.** + +Take a device pulled after the reset completes and another plugged into the +same port before that read. On a USB3 port the new device trains to Enabled by +itself (§4.19.1.2). The CSC its arrival raised is the one +`enumeration_ack(Some(Warm), …)` spends, because a warm reset's retrain raises +one too (§4.19.5.1) and nothing tells the two apart. A second unit of the same +model answers every step: every identity field is the same but its serial +number, as with `usb_transport_break`'s `AnotherStick`. The rung then carries +disk 0's volume on onto it. + +The port machine does not see the swap either. Its belief stays attached with +disk 0's slot, and the rung's acknowledge spent the edge. + +`usb_transport_break` moves the stick to port 3, so the bind's serial check +(`XhciController::adopt`) judges it there. Nothing stages the same port. + +**Exit**: a disk the port rung took back is the device it was. Its serial +number is read again and judged by `toyos_xhci::identity::same` before the +volume carries on. A `usb-reset-moves` staging that plugs another serial number +into the same port refuses it by name. diff --git a/issues/kernel/a-held-disk-waits-for-a-pass-no-cpu-takes-when-every-cpu-is-in-a-call-on-it.md b/issues/kernel/a-held-disk-waits-for-a-pass-no-cpu-takes-when-every-cpu-is-in-a-call-on-it.md index d9bfc536b7..3dd0a884f9 100644 --- a/issues/kernel/a-held-disk-waits-for-a-pass-no-cpu-takes-when-every-cpu-is-in-a-call-on-it.md +++ b/issues/kernel/a-held-disk-waits-for-a-pass-no-cpu-takes-when-every-cpu-is-in-a-call-on-it.md @@ -23,6 +23,25 @@ throughout and was first enumerated at 2.455 s, after both calls ended at untaken, so the disk is taken back and the retried operations complete (1 of 6 runs at `f0695038` took this shape and passed). +**A CPU spinning on a lock the call's caller holds counts too.** +`usb_transport_break --nightly` at `e889d03e` (#554), the `AnotherStick` boot +(`554r6-usb_transport_break.log` in the job scratchpad), red. cpu0 spent 0.383 +to 4.385 s in logd's create of the boot's log file, which holds `vfs::lock()` +(`object::ops::open`): two writes of block 9351 on held disk 0, each ending +`still held` on its bound, at 2.384 and 4.385, with no pass between them. +cpu1 logged nothing from 0.311 to 4.390 s. Two threads resumed within a +millisecond of that create's end: `test-runner`'s spawn of `reboot`, whose own +`total=7ms` puts its start at about 4.383, and init's `started test-runner` +line, stamped 4.386 for a spawn made at 0.363. An idle cpu1 kicked by +`wait_for_return` would have torn port 1 down within its 100 ms debounce, and +no `port 1 disconnected` line exists, so cpu1 took no pass. That it spun on the +VFS lock is inferred, not measured. Port 3 read connected and untaken +throughout, so the arrival rule kept disk 0 held and no `did not come back` +line came either. The other stick was never enumerated, and the test's +`is not disk 0 come back` line never came. From 4.424 s cpu0 spun in the +reboot's sync and cpu1 in a shootdown +(`a-shutdown-on-a-held-usb-disk-left-a-cpu-deaf-to-a-tlb-shootdown.md`). + **What is still wrong**: the operation that waited answers `BudgetExpired` instead of the device's answer, and a caller that gives up on one — `logd` on a refused create (`issues/boot-media/logd-ends-the-boots-log-on-one-refused-create-and-nothing-durable-says-so.md`) diff --git a/issues/kernel/a-shutdown-on-a-held-usb-disk-left-a-cpu-deaf-to-a-tlb-shootdown.md b/issues/kernel/a-shutdown-on-a-held-usb-disk-left-a-cpu-deaf-to-a-tlb-shootdown.md index ea42025699..ecf2fd3016 100644 --- a/issues/kernel/a-shutdown-on-a-held-usb-disk-left-a-cpu-deaf-to-a-tlb-shootdown.md +++ b/issues/kernel/a-shutdown-on-a-held-usb-disk-left-a-cpu-deaf-to-a-tlb-shootdown.md @@ -6,7 +6,7 @@ opened: 2026-09-24 # A shutdown on a held USB disk left a CPU deaf to a TLB shootdown for five seconds, and the kernel panicked -One sighting, `usb_transport_break` in a local `--nightly` run on the logd +`usb_transport_break` in a local `--nightly` run on the logd branch at `eef19bd1` (parameter line `root=…,usb-transport-break,usb-reset-moves,blackbox=0x8000000`, two CPUs, TCG). Alone it was green. @@ -34,6 +34,16 @@ the arithmetic for one disk operation outrunning `time::DEAF_CPU`; this is a boo that did outrun it, in `quiesce`, across several operations each inside its own budget. Whether `quiesce` holds `IF` clear between them is not measured. +**Second sighting, with the roles swapped**: `usb_transport_break --nightly` at +`e889d03e` (#554), the `AnotherStick` boot (`554r6-usb_transport_break.log` in +the job scratchpad). cpu0 took the reboot's `Syncing filesystems` at 4.424 s +and spent it in calls on held disk 0, each ending `still held`: at 6.427, 8.428 +and 10.430. cpu1, in its idle loop, dropped an `InboxRef` in +`object::drain_zero_handles` and waited on cpu0: + + [kernel 9.425 cpu1] PANIC: panicked at src/arch/x86_64/tlb.rs:151:42: + tlb: cpu 0 has not flushed for generation Generation(1) in 5000000000ns — it is not taking interrupts + ## Exit condition What holds cpu1's interrupts off across that window is named from a boot, and From c1d9b4fba003aac1d289531f52edd7743dbf5764 Mon Sep 17 00:00:00 2001 From: japabu Date: Mon, 28 Sep 2026 09:02:52 +0200 Subject: [PATCH 11/11] xHCI: a warm ResetFailed becomes unrepresentable GaveUp::ResetNeverFinished and GaveUp::ResetFailed only ever held Reset::Hot: reset_outcome and never_finished map every warm end to LinkNeverTrained. Drop the dead Reset payload from both, delete GaveUp::reset (its only caller), and inline the "hot" the two kernel log arms and boot's chose from it: a warm ResetFailed is now unrepresentable rather than merely unreached. gave_up's slot argument at the hot arm was always None, since Resetting is entered only from a port believed empty; write None rather than self.slot. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01W6rME2DoqwjcYFStYHHY4j --- kernel/src/drivers/xhci/mod.rs | 16 ++++------------ kernel/src/drivers/xhci/wait/boot.rs | 7 +++---- toyos-xhci/sim/tests/scenarios.rs | 2 +- toyos-xhci/sim/tests/superspeed.rs | 4 ++-- toyos-xhci/src/port.rs | 26 +++++++++----------------- 5 files changed, 19 insertions(+), 36 deletions(-) diff --git a/kernel/src/drivers/xhci/mod.rs b/kernel/src/drivers/xhci/mod.rs index 07da6dad02..d2b4fd8992 100644 --- a/kernel/src/drivers/xhci/mod.rs +++ b/kernel/src/drivers/xhci/mod.rs @@ -1241,14 +1241,10 @@ impl XhciController { Step::Wait(at) => return Some(at), Step::GaveUp(why) => { match why { - GaveUp::ResetNeverFinished(kind) => log!( - "xHCI: port {} never finished its {} reset (PORTSC {:#010x}); \ + GaveUp::ResetNeverFinished => log!( + "xHCI: port {} never finished its hot reset (PORTSC {:#010x}); \ skipping it", port_idx + 1, - match kind { - Reset::Hot => "hot", - Reset::Warm => "warm", - }, portsc.raw() ), // §4.19.1.2 has nothing further after a warm reset — this is the port's end. @@ -1259,14 +1255,10 @@ impl XhciController { portsc.raw(), portsc.link_state() ), - GaveUp::ResetFailed(kind) => log!( - "xHCI: port {} completed its {} reset without enabling \ + GaveUp::ResetFailed => log!( + "xHCI: port {} completed its hot reset without enabling \ (PORTSC {:#010x}); skipping it", port_idx + 1, - match kind { - Reset::Hot => "hot", - Reset::Warm => "warm", - }, portsc.raw() ), } diff --git a/kernel/src/drivers/xhci/wait/boot.rs b/kernel/src/drivers/xhci/wait/boot.rs index 686dc9edbf..d56866fa65 100644 --- a/kernel/src/drivers/xhci/wait/boot.rs +++ b/kernel/src/drivers/xhci/wait/boot.rs @@ -482,13 +482,12 @@ pub fn init_device(ctrl: &mut XhciController, port_idx: u8, protocol: Option log!( - "xHCI: port {} completed its {} reset without enabling \ + GaveUp::ResetFailed => log!( + "xHCI: port {} completed its hot reset without enabling \ (PORTSC {:#010x}); skipping it", port_idx + 1, - match k { Reset::Hot => "hot", Reset::Warm => "warm" }, ctrl.read_portsc(port_idx).raw()), - GaveUp::ResetNeverFinished(_) => { + GaveUp::ResetNeverFinished => { unreachable!("a completed reset cannot have never finished") } } diff --git a/toyos-xhci/sim/tests/scenarios.rs b/toyos-xhci/sim/tests/scenarios.rs index 60ee264c35..e350b16ac3 100644 --- a/toyos-xhci/sim/tests/scenarios.rs +++ b/toyos-xhci/sim/tests/scenarios.rs @@ -94,7 +94,7 @@ fn a_port_that_never_finishes_its_reset_is_given_up_on_and_left_alone() { driver .run_to(&mut port, 0, DEBOUNCE_NS + RESET_DEADLINE_NS + PASS, PASS) .unwrap(); - assert_eq!(driver.did, [Did::Reset(Reset::Hot), Did::GaveUp(GaveUp::ResetNeverFinished(Reset::Hot))], "{:?}", driver.did); + assert_eq!(driver.did, [Did::Reset(Reset::Hot), Did::GaveUp(GaveUp::ResetNeverFinished)], "{:?}", driver.did); // And it stays given up on: a port retried every pass is a port that costs // the machine a reset per pass for as long as the device stays in it. diff --git a/toyos-xhci/sim/tests/superspeed.rs b/toyos-xhci/sim/tests/superspeed.rs index c7525e53ce..4a0ba478cf 100644 --- a/toyos-xhci/sim/tests/superspeed.rs +++ b/toyos-xhci/sim/tests/superspeed.rs @@ -141,7 +141,7 @@ fn a_usb2_port_is_never_warm_reset() { assert_eq!(driver.resets(), [Reset::Hot], "{:?}", driver.did); assert_eq!( driver.did.last(), - Some(&Did::GaveUp(GaveUp::ResetNeverFinished(Reset::Hot))), + Some(&Did::GaveUp(GaveUp::ResetNeverFinished)), "{:?}", driver.did ); @@ -302,7 +302,7 @@ fn a_usb2_completed_failure_is_refused_not_warm_reset() { .unwrap(); assert_eq!(driver.resets(), [Reset::Hot], "{:?}", driver.did); assert!( - driver.did.contains(&Did::GaveUp(GaveUp::ResetFailed(Reset::Hot))), + driver.did.contains(&Did::GaveUp(GaveUp::ResetFailed)), "{:?}", driver.did ); diff --git a/toyos-xhci/src/port.rs b/toyos-xhci/src/port.rs index deadd65bff..43afdcb6ab 100644 --- a/toyos-xhci/src/port.rs +++ b/toyos-xhci/src/port.rs @@ -134,16 +134,16 @@ pub fn due(signalled: bool, ports: &[PortState]) -> bool { /// Why a port stopped being worked on. #[derive(Clone, Copy, PartialEq, Eq, Debug)] pub enum GaveUp { - /// A reset was written and no completion came. - ResetNeverFinished(Reset), + /// A hot reset was written and no completion came. + ResetNeverFinished, /// A USB3 link was warm-reset as well and still did not come up. /// §4.19.1.2 has nothing beyond a warm reset, so this is the end of the /// road for the port rather than one step short of it. LinkNeverTrained, - /// The reset *completed* with the port still disabled, where §4.19.5 + /// The hot reset *completed* with the port still disabled, where §4.19.5 /// offers no escalation ("USB2 protocol ports never fail"): a controller /// misbehaving, not a link a warm reset could retrain. - ResetFailed(Reset), + ResetFailed, } impl GaveUp { @@ -151,15 +151,7 @@ impl GaveUp { pub fn never_finished(kind: Reset) -> Self { match kind { Reset::Warm => GaveUp::LinkNeverTrained, - Reset::Hot => GaveUp::ResetNeverFinished(Reset::Hot), - } - } - - /// The reset that was given up on. - fn reset(self) -> Reset { - match self { - GaveUp::ResetNeverFinished(kind) | GaveUp::ResetFailed(kind) => kind, - GaveUp::LinkNeverTrained => Reset::Warm, + Reset::Hot => GaveUp::ResetNeverFinished, } } } @@ -180,7 +172,7 @@ pub fn reset_outcome(kind: Reset, protocol: Option, portsc: Portsc) -> } ResetOutcome::GaveUp(match kind { Reset::Warm => GaveUp::LinkNeverTrained, - Reset::Hot => GaveUp::ResetFailed(Reset::Hot), + Reset::Hot => GaveUp::ResetFailed, }) } @@ -426,18 +418,18 @@ impl PortState { /// The port was given up on: **attached, so it is not reset again until a /// fresh edge moves it**, and read once, so a pull is still seen. pub fn gave_up(&mut self, why: GaveUp) { - match why.reset() { + match why { // §4.19.5.1's retrain raises a connect edge of its own, which // nothing tells apart from a replug; judged as one, it would tear // the port down and reset it again every debounce for as long as // its device stayed in. - Reset::Warm => { + GaveUp::LinkNeverTrained => { self.attached = true; self.work = Work::GivenUp; } // A hot reset changes no connect state, so a connect flag is a // real replug. - Reset::Hot => self.believe(true, self.slot), + GaveUp::ResetNeverFinished | GaveUp::ResetFailed => self.believe(true, None), } }