Implement event data trb handling by Noi0103 · Pull Request #238 · cyberus-technology/usbvfiod

Noi0103 · 2026-05-06T12:55:33Z

Windows is never (as far as I saw in logs at least) utilizing the interrupt on completion bit. Windows will instead use event data trb.

The event data appeared on the Control Endpoint and the handler is initially written for the Control Endpoint.

When NormalIn and NormalOut Endpoints also needed event data handling I vendored the handle functions into the respective NormalEndpoint implementations because it seemed easiest. Combining this so that any endpoint can call the same handle_event_data seems better/less error prone.
Should that be added to this PR?

snue

Did a partial review and left some comments. I really like the added unit tests.

Most importantly I'd like to get rid of most of the unreachable!() statements through code refactoring. And we also need to clarify what we do about the device overread / TD handling.

I also left lots of nits about naming and such. You really like long redundant variable names 😸

snue · 2026-07-06T17:15:06Z

+
+    let event_data_trb_data = match &transfer_trb.variant {
+        TransferTrbVariant::EventData(event_data_trb_data) => event_data_trb_data,
+        _ => unreachable!("checked variant before calling this handle"),


This redundant naming is exhausting.

Suggested change

_ => unreachable!("checked variant before calling this handle"),

let event_data = match &transfer_trb.variant {

TransferTrbVariant::EventData(data) => data,

Or even better, as you have all these unreachable!() branches in the called functions, can we modify the signature to take the embedded data directly and fix this in the caller?

snue

I have a bunch of comments stored up. We are getting there, but I think you missed treating some of the error paths correctly? Could these be caught by unit tests as well?

A lot of my comments are nits for the documentation or name suggestions. I see some of my previously suggested renames are for pre-existing data structures (dropping the redundant Data part). I still think these are better names, but obviously it makes the diff larger. Sorry for that.

There are also some comments on outdated sections that I can't figure out how to edit/remove through the GitHub UI.

Noi0103 · 2026-07-13T13:36:41Z

Tests with a DummyControlEndpoint that would return specific errors are not yet included.

This enables a Data Stage to be composed of more than a single DataStageTrb.

Allow Event Data Trb and handle them accordingly when they are expected according to the spec.

The Event Data handle function has strong similarities between Control and each Normal Endpoint.

To use assert!() and similar derives have been added. Access to TransferEventTrbData were previously not necessary and are only public to enable the tests in the following commit. The MockInterrupter is using an async function to avoid having to implement any `wait_until_succeeds()` like utility in future tests where we might otherwise get race conditions between tasks. To better differentiate between release and test code the test only implementations are named Mock, because there are Dummy implementations used in the application.

The ControlEndpoint is tested with variations of trb input sequences with and without Event Data TRB. The NormalInEnpoint and NormalOutEndpoint are tested with a longer sequence each, including a section without Event Data TRB and a section with Event Data TRB.

snue

I have not done a full review, but it looks like we are getting there. The remaining unreachable statements are well explained and can be trusted. Left some comments, found typos etc.

snue · 2026-07-17T16:32:57Z

                        request: request.request,
                        value: request.value,
                        index: request.index,
                        data: &data,


Suggested change

data: &request.data,

Instead of the let data above?

snue · 2026-07-17T16:38:08Z

+    }
+    /// input is 17 Bit
+    const fn add(&mut self, addend: u32) {
+        self.edtla = MAX_VALUE_U24 & (self.edtla.wrapping_add(addend));


The addend name confuses me 😅

Suggested change

self.edtla = MAX_VALUE_U24 & (self.edtla.wrapping_add(addend));

self.edtla = MAX_VALUE_U24 & (self.edtla.wrapping_add(transfered_bytes));

snue · 2026-07-17T16:38:49Z

+    event_meta: EventDataTrbMetadata,
+}
+impl ControlTransferState {
+    // previous_completion_code should never be used as is, thus the error as a default value


This is a remnant?

snue · 2026-07-17T16:45:53Z

+                    let mut tmp = vec![0u8; transfer_length as usize];
+                    self.dma_bus.read_bulk(data_pointer, &mut tmp);
+
+                    // SAFETY: is always set in the preceding setup stage


This is actually safe now.

snue · 2026-07-17T16:47:29Z

+                    self.transfer_state.event_meta.zero();
+                }
+                _ => {
+                    error!("driver did not provide a spec ocmpliant control transfer trb chain");


Suggested change

error!("driver did not provide a spec ocmpliant control transfer trb chain");

error!("driver did not provide a spec compliant control transfer trb chain");

snue · 2026-07-17T17:03:01Z

+
+        let completion_code: CompletionCode;
+
+        // SACETY: in case the hardware_data.len() is bigger than u32 we take the u32 value regardless


typo:

Suggested change

// SACETY: in case the hardware_data.len() is bigger than u32 we take the u32 value regardless

// SAFETY: in case the hardware_data.len() is bigger than u32 we take the u32 value regardless

snue · 2026-07-17T17:09:23Z

+    fn handle_status_stage_trb_hardware_response(
+        &mut self,
+        address: u64,
+        trb: StatusStageTrb,
+    ) -> anyhow::Result<()> {
+        match &mut self.transfer_state.data {
+            ControlTransferData::In(_) => {
+                unreachable!("internal error: ControlIn requests do the Hardware request in the SetupStage; a submission state to arrive here should never be used");
+            }


Ah, we put so much effort into ensuring you can only call this with the right kind of TRB, but I think it would make much more sense if we had a single handle_hardware_response function that does the right thing depending on In/Out, instead of splitting it and having unreachable! cases in the respective functions. I may be missing something here, though.

Noi0103 marked this pull request as ready for review May 6, 2026 14:31

Noi0103 requested review from lbeierlieb and snue May 6, 2026 14:31

Noi0103 force-pushed the Noi0103/implement-event-data-trb-handling branch 3 times, most recently from 5c81728 to bcd3d40 Compare May 8, 2026 12:30

Noi0103 force-pushed the Noi0103/implement-event-data-trb-handling branch from bcd3d40 to f46cde9 Compare June 1, 2026 12:53

Noi0103 marked this pull request as draft June 1, 2026 13:26

Noi0103 force-pushed the Noi0103/implement-event-data-trb-handling branch 8 times, most recently from a96a6d4 to 35f5464 Compare June 8, 2026 09:40

Noi0103 marked this pull request as ready for review June 8, 2026 11:57

Noi0103 force-pushed the Noi0103/implement-event-data-trb-handling branch 4 times, most recently from 1f871b2 to 5e28c9f Compare June 23, 2026 15:30

Noi0103 force-pushed the Noi0103/implement-event-data-trb-handling branch 4 times, most recently from 67003f1 to 8067893 Compare June 30, 2026 15:37

Noi0103 self-assigned this Jul 1, 2026

Noi0103 force-pushed the Noi0103/implement-event-data-trb-handling branch from 8067893 to e0da4a4 Compare July 6, 2026 07:35

trb: derive clone for trb data

869d160

Noi0103 force-pushed the Noi0103/implement-event-data-trb-handling branch from e0da4a4 to 6b22267 Compare July 6, 2026 07:48

snue reviewed Jul 6, 2026

View reviewed changes

Noi0103 force-pushed the Noi0103/implement-event-data-trb-handling branch from 6b22267 to 812139c Compare July 8, 2026 11:21

snue reviewed Jul 8, 2026

View reviewed changes

Comment thread src/device/xhci/trb.rs Outdated

Comment thread src/device/xhci/trb.rs Outdated

snue reviewed Jul 8, 2026

View reviewed changes

Comment thread src/device/xhci/endpoint_handle.rs Outdated

trb: refactor parsed trb structs and fix DataStageTrb

4ae77b7

Noi0103 force-pushed the Noi0103/implement-event-data-trb-handling branch from 812139c to a2e9a51 Compare July 10, 2026 14:43

snue reviewed Jul 10, 2026

View reviewed changes

endpoint_handle: refactor usbrequest to remove option wrapper on data

e3aedd1

Noi0103 force-pushed the Noi0103/implement-event-data-trb-handling branch from a2e9a51 to 5a603d6 Compare July 13, 2026 13:35

Noi0103 force-pushed the Noi0103/implement-event-data-trb-handling branch 2 times, most recently from f302339 to efe1749 Compare July 14, 2026 08:56

Noi0103 requested a review from snue July 14, 2026 11:06

Noi0103 force-pushed the Noi0103/implement-event-data-trb-handling branch 2 times, most recently from a7c7300 to aeb5223 Compare July 15, 2026 10:06

Noi0103 added 6 commits July 17, 2026 13:26

endpoint_handle: control request with multiple trb in data stage td

945813e

This enables a Data Stage to be composed of more than a single DataStageTrb.

endpont_handle: handle EventDataTrb in Control Requests

6a4c12e

Allow Event Data Trb and handle them accordingly when they are expected according to the spec.

endpoint_handle: add state machine mermaid diagram

11fac83

endpoint_handle: implement event data for both Normal Endpoints

e64e1cb

The Event Data handle function has strong similarities between Control and each Normal Endpoint.

Noi0103 force-pushed the Noi0103/implement-event-data-trb-handling branch from aeb5223 to 8b7b352 Compare July 17, 2026 11:26

snue reviewed Jul 17, 2026

View reviewed changes

	_ => unreachable!("checked variant before calling this handle"),
	let event_data = match &transfer_trb.variant {
	TransferTrbVariant::EventData(data) => data,

	self.edtla = MAX_VALUE_U24 & (self.edtla.wrapping_add(addend));
	self.edtla = MAX_VALUE_U24 & (self.edtla.wrapping_add(transfered_bytes));

	error!("driver did not provide a spec ocmpliant control transfer trb chain");
	error!("driver did not provide a spec compliant control transfer trb chain");


		let completion_code: CompletionCode;

		// SACETY: in case the hardware_data.len() is bigger than u32 we take the u32 value regardless

	// SACETY: in case the hardware_data.len() is bigger than u32 we take the u32 value regardless
	// SAFETY: in case the hardware_data.len() is bigger than u32 we take the u32 value regardless

Uh oh!

Conversation

Noi0103 commented May 6, 2026

Uh oh!

snue left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

snue left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Noi0103 commented Jul 13, 2026

Uh oh!

snue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants