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 4fbcd4a15d7..00000000000 --- a/issues/hardware/a-collapsed-replug-is-enumerated-only-when-another-port-event-arrives.md +++ /dev/null @@ -1,74 +0,0 @@ ---- -status: expected-red -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. 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 00000000000..3d76a6a42ec --- /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-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 00000000000..a115d929cf7 --- /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-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 df18b1914ba..4cd36d5b146 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-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 00000000000..c8e2419afa7 --- /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-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 ea42025699d..ecf2fd3016e 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 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 00000000000..44556c82b01 --- /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 dc9fb0edf63..5b9dbb4185d 100644 --- a/kernel/src/drivers/xhci/mod.rs +++ b/kernel/src/drivers/xhci/mod.rs @@ -1191,11 +1191,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. @@ -1245,14 +1240,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. @@ -1263,18 +1254,14 @@ 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() ), } - 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) => { @@ -1319,8 +1306,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. } } } @@ -1525,7 +1511,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 493b58fb362..d56866fa656 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,8 +139,6 @@ 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); 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()); @@ -161,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; } @@ -484,17 +482,16 @@ 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") } } - return ctrl.port_bound(port_idx, None); + return ctrl.ports[usize::from(port_idx)].gave_up(why); } } } @@ -509,7 +506,7 @@ pub fn init_device(ctrl: &mut XhciController, port_idx: u8, protocol: 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 && bound(line) { + 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,26 +3974,31 @@ 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().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) => { + "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 - // 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}" )); } @@ -4028,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 \ diff --git a/toyos-xhci/sim/src/driver.rs b/toyos-xhci/sim/src/driver.rs index 3261ff32b3e..0f19ebddad0 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. @@ -121,6 +125,7 @@ pub struct Driver { /// an enumeration would do. The enumeration drains the event ring, so this /// is reachable rather than hypothetical. reenter: bool, + 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, @@ -178,6 +183,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, @@ -236,6 +242,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 { @@ -310,10 +323,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 +366,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)); @@ -390,6 +411,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)); } @@ -405,6 +429,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/src/hub.rs b/toyos-xhci/sim/src/hub.rs index 9d538e65f35..d08604eee73 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; @@ -45,6 +47,11 @@ pub enum ResetBehaviour { /// comes with the port disabled, CCS and speed zero, the link at /// RxDetect; §4.19.5.1's warm reset is the prescribed recovery. FailsTheBusReset { warm_works: bool }, + /// **A USB3 link a retrain finds and cannot bring up.** The bus reset fails + /// as [`Self::FailsTheBusReset`]'s does, and the warm reset completes having + /// re-detected the device — PRC, WRC, CCS and CSC — with the port still + /// disabled. + RetrainsDisabled, } pub struct FakePort { @@ -64,6 +71,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 +86,7 @@ impl FakePort { present: false, superspeed: false, writes: Vec::new(), + signalled: false, } } @@ -97,6 +107,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 +125,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 +137,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 +173,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 +192,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; } @@ -185,46 +201,51 @@ impl FakePort { } 1_000_000 } - ResetBehaviour::FailsTheBusReset { warm_works } => { - if !self.warm { - // §4.19.5's completed failure, bit for bit. - self.resetting_since = None; - self.raw &= !(PR | PED | (0xF << SPEED_SHIFT)); - if self.raw & CCS != 0 { - self.raw = (self.raw & !CCS) | CSC; - } - 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; } 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); + 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); } } 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 b3283dbd872..c64310ef643 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/sim/tests/scenarios.rs b/toyos-xhci/sim/tests/scenarios.rs index 4737cdce3b7..e350b16ac31 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. @@ -110,6 +110,22 @@ fn a_port_that_never_finishes_its_reset_is_given_up_on_and_left_alone() { assert_eq!(driver.did.len(), before, "the port was tried again: {:?}", driver.did); } +/// A hot reset raises no connect edge of its own, so the connect flag a hot +/// give-up leaves is a real replug, and what is in the port now is enumerated. +#[test] +fn a_device_replugged_inside_a_hot_reset_given_up_on_is_enumerated() { + let mut port = FakePort::occupied(ResetBehaviour::Completes { after: 50_000_000 }); + let mut driver = Driver::new(); + driver.run_to(&mut port, 0, DEBOUNCE_NS + 2 * PASS, PASS).unwrap(); + assert_eq!(driver.did, [Did::Reset(Reset::Hot)]); + // The detach ends the reset; the attach finds CSC already set. + port.replug(); + driver + .run_to(&mut port, DEBOUNCE_NS + 2 * PASS, 2 * RESET_DEADLINE_NS + 4 * DEBOUNCE_NS, PASS) + .unwrap(); + assert_eq!(driver.enumerations(), 1, "{:?}", driver.did); +} + /// Four replugs, which is a person fidgeting with a cable. Each must produce /// exactly one teardown and one enumeration, and the port must end bound. #[test] diff --git a/toyos-xhci/sim/tests/superspeed.rs b/toyos-xhci/sim/tests/superspeed.rs index 16eb850e00e..4a0ba478cf6 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 ); @@ -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] @@ -234,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] @@ -245,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 8adf21015a8..43afdcb6ab5 100644 --- a/toyos-xhci/src/port.rs +++ b/toyos-xhci/src/port.rs @@ -121,19 +121,39 @@ 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 { - /// 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 { + /// A `kind` reset whose deadline passed with no completion. + pub fn never_finished(kind: Reset) -> Self { + match kind { + Reset::Warm => GaveUp::LinkNeverTrained, + Reset::Hot => GaveUp::ResetNeverFinished, + } + } } /// What a completed reset means for the port — **the one place that question @@ -152,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, }) } @@ -263,6 +283,12 @@ enum Work { Resetting { until: Nanos, kind: Reset }, /// The caller is inside an effect and has not reported it. Working(Effect), + /// The driver's belief moved ([`PortState::believe`]) and nothing has read + /// the register against it since. + Unread, + /// A warm reset was given up on ([`PortState::gave_up`]) and no read has + /// found the port's change flags clear since. + GivenUp, } /// A deliberate defect, compiled only for the negative gates. @@ -361,20 +387,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, 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::Settled; + 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. @@ -382,10 +404,38 @@ impl PortState { self.protocol = protocol; } - pub fn adopt(&mut self, slot: Option) { - self.attached = true; + /// **Leaves the port [`Self::outstanding`] until a look has read the + /// 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. + fn believe(&mut self, attached: bool, slot: Option) { + self.attached = attached; self.slot = slot; - self.work = Work::Settled; + self.work = Work::Unread; + } + + /// 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 { + // §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. + GaveUp::LinkNeverTrained => { + self.attached = true; + self.work = Work::GivenUp; + } + // A hot reset changes no connect state, so a connect flag is a + // real replug. + GaveUp::ResetNeverFinished | GaveUp::ResetFailed => self.believe(true, None), + } + } + + fn give_up(&mut self, why: GaveUp) -> Step<'static> { + self.gave_up(why); + Step::GaveUp(why) } #[cfg(feature = "flaws")] @@ -437,11 +487,7 @@ impl PortState { Work::Resetting { until: now + RESET_DEADLINE_NS, kind: Reset::Warm }; return Step::Reset(Reset::Warm, write); } - ResetOutcome::GaveUp(why) => { - self.attached = true; - self.work = Work::Settled; - return Step::GaveUp(why); - } + ResetOutcome::GaveUp(why) => return self.give_up(why), } } if now < until || self.flawed(Flaw::NoResetDeadline) { @@ -455,15 +501,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.attached = true; - self.work = Work::Settled; - return Step::GaveUp(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() { @@ -477,7 +515,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)); } @@ -494,8 +532,9 @@ impl PortState { } let held = match self.work { - Work::Settled => { + Work::Settled | Work::Unread | Work::GivenUp => { if connected == self.attached { + self.work = Work::Settled; return Step::Idle; } self.work = Work::Debouncing { at: now }; @@ -586,6 +625,29 @@ mod tests { assert_eq!(inherited(None, trained), Reset::Hot); } + /// 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_port_whose_belief_moved_is_outstanding_until_it_is_read() { + let empty = Portsc::from_raw(1 << 9); + let reports = [ + ("enumerated", (|p| p.enumerated(NonZeroU8::new(1))) as fn(&mut PortState)), + ("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] 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);