From 260003331ed78736a033b3dce645c36935a1e0be Mon Sep 17 00:00:00 2001 From: Red Bear OS Date: Sat, 18 Jul 2026 23:14:14 +0900 Subject: [PATCH] xhcid: fix restart_endpoint deadlock, doorbell ordering, NoOp priming (P2-C) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three bugs in the xHCI endpoint-restart path used by all error recovery (stall hard reset, transaction-error soft retry, resource retry, split/babble hard reset): 1. Latent deadlock: restart_endpoint held the port_states write guard across set_tr_deque_ptr(), which internally re-acquires a read guard on the same key (std RwLock read-while-write on one thread). Unobserved because error injection is not yet exercised at runtime (P8-C). Fixed by scoping phase-1 ring priming so the guard drops before the async command. 2. Doorbell ordering violated xHCI spec 4.6.8/4.6.10: after Reset Endpoint the TR Dequeue Pointer is undefined, so Set TR Dequeue Pointer must complete BEFORE the doorbell transitions the endpoint Stopped->Running. The old order (doorbell first) ran the endpoint with an undefined dequeue pointer — undefined xHC behavior on real hardware. Linux rings the doorbell from the Set TR Dequeue command completion path (xhci_handle_cmd_set_deq, ring.c:1416-1554); xhcid now issues Set TR Dequeue, awaits completion, then rings. 3. Priming NoOp never executed: ring.register() was captured after ring.next() advanced the enqueue index, so the dequeue pointed past the NoOp (dead TRB). Now captured before next(), priming the hardware dequeue AT the NoOp so the xHC executes it on restart — same semantics as Linux xhci_move_dequeue_past_td pointing at the first valid TRB. Verified: cargo check clean (138 warnings, unchanged), 43/43 tests pass. --- drivers/usb/xhcid/src/xhci/scheme.rs | 103 ++++++++++++++++----------- 1 file changed, 62 insertions(+), 41 deletions(-) diff --git a/drivers/usb/xhcid/src/xhci/scheme.rs b/drivers/usb/xhcid/src/xhci/scheme.rs index 968a2eb837..e676355289 100644 --- a/drivers/usb/xhcid/src/xhci/scheme.rs +++ b/drivers/usb/xhcid/src/xhci/scheme.rs @@ -3042,54 +3042,75 @@ impl Xhci { Ok(()) } pub async fn restart_endpoint(&self, port_num: PortId, endp_num: u8) -> Result<()> { - let mut port_state = self - .port_states - .get_mut(&port_num) - .ok_or(Error::new(EBADFD))?; - let slot = port_state.slot; + // Phase 1 (lock held): prime the transfer ring with a NoOp and + // compute the doorbell value. The port_states write guard MUST be + // dropped before the set_tr_deque_ptr() await below, because that + // function re-acquires a read guard on the same key — holding the + // write guard across it deadlocks (std RwLock read-while-write on + // one thread). + let (slot, deque_ptr_and_cycle, doorbell) = { + let mut port_state = self + .port_states + .get_mut(&port_num) + .ok_or(Error::new(EBADFD))?; + let slot = port_state.slot; - let mut endpoint_state = port_state - .endpoint_states - .get_mut(&endp_num) - .ok_or(Error::new(EBADFD))?; + let mut endpoint_state = port_state + .endpoint_states + .get_mut(&endp_num) + .ok_or(Error::new(EBADFD))?; - let (has_streams, ring) = match &mut endpoint_state.transfer { - &mut super::RingOrStreams::Ring(ref mut ring) => (false, ring), - &mut super::RingOrStreams::Streams(ref mut arr) => { - (true, arr.rings.get_mut(&1).ok_or(Error::new(EBADFD))?) - } + let (has_streams, ring) = match &mut endpoint_state.transfer { + &mut super::RingOrStreams::Ring(ref mut ring) => (false, ring), + &mut super::RingOrStreams::Streams(ref mut arr) => { + (true, arr.rings.get_mut(&1).ok_or(Error::new(EBADFD))?) + } + }; + + // Capture the NoOp's own address BEFORE advancing the enqueue + // index, so Set TR Dequeue Pointer primes the hardware dequeue + // AT the NoOp — the xHC executes it on restart, proving the + // ring is live again (Linux xhci_move_dequeue_past_td primes + // the dequeue at the first valid TRB the same way). + let deque_ptr_and_cycle = ring.register(); + let (cmd, cycle) = ring.next(); + cmd.transfer_no_op(0, false, false, false, cycle); + + let dev_desc = port_state.dev_desc.as_ref().ok_or(Error::new(EBADFD))?; + let endp_desc = dev_desc + .config_descs + .get(0) + .ok_or(Error::new(EIO))? + .interface_descs + .get(0) + .ok_or(Error::new(EIO))? + .endpoints + .get(endp_num as usize - 1) + .ok_or(Error::new(EBADFD))?; + + let doorbell = if endp_num != 0 { + let stream_id = 1u16; + + Self::endp_doorbell(endp_num, endp_desc, if has_streams { stream_id } else { 0 }) + } else { + Self::def_control_endp_doorbell() + }; + (slot, deque_ptr_and_cycle, doorbell) }; - let (cmd, cycle) = ring.next(); - cmd.transfer_no_op(0, false, false, false, cycle); - - let deque_ptr_and_cycle = ring.register(); - - let dev_desc = port_state.dev_desc.as_ref().ok_or(Error::new(EBADFD))?; - let endp_desc = dev_desc - .config_descs - .get(0) - .ok_or(Error::new(EIO))? - .interface_descs - .get(0) - .ok_or(Error::new(EIO))? - .endpoints - .get(endp_num as usize - 1) - .ok_or(Error::new(EBADFD))?; - - let doorbell = if endp_num != 0 { - let stream_id = 1u16; - - Self::endp_doorbell(endp_num, endp_desc, if has_streams { stream_id } else { 0 }) - } else { - Self::def_control_endp_doorbell() - }; - - self.dbs.lock().unwrap_or_else(|e| e.into_inner())[slot as usize].write(doorbell); - + // Phase 2 (lock released): move the hardware dequeue pointer, then + // ring the doorbell. xHCI spec 4.6.8/4.6.10: after Reset Endpoint + // the TR Dequeue Pointer is undefined, so Set TR Dequeue Pointer + // MUST complete before the doorbell transitions the endpoint + // Stopped→Running. Linux rings the doorbell from the Set TR + // Dequeue completion path (xhci_handle_cmd_set_deq) for the same + // reason. The previous order (doorbell first) ran the endpoint + // with an undefined dequeue pointer — undefined xHC behavior. self.set_tr_deque_ptr(port_num, endp_num, deque_ptr_and_cycle) .await?; + self.dbs.lock().unwrap_or_else(|e| e.into_inner())[slot as usize].write(doorbell); + Ok(()) } pub fn endp_direction(&self, port_num: PortId, endp_num: u8) -> Result {