Skip to content

[test] fixes spurious mailbox/USB stalls during pre-sleep handshake - #2923

Merged
technobly merged 1 commit into
developfrom
fix/test-runner-mailbox-usb-stall
May 27, 2026
Merged

technobly merged 1 commit into
developfrom
fix/test-runner-mailbox-usb-stall

Conversation

@technobly

@technobly technobly commented May 23, 2026

Copy link
Copy Markdown
Member

Problem

  • The wiring/sleep20 suite intermittently failed on Linux test rigs (kernel 6.8 / AMD Rembrandt USB4 XHCI) with the device-side pushMailbox returning success while the host-side runner observed a USB control-transfer stall on
    the corresponding mailbox read. The device would then race into System.sleep and tear down USB before the runner could re-issue the read, de-syncing the suite and triggering a fixture-reset cascade.
  • Root cause is a kernel-level xhci_hcd bug: the driver synthesizes -EPIPE (LIBUSB_TRANSFER_STALL) on control IN URBs even when the wire-level data stage completed and was ACKed by host hardware. Empirically, the stalls cluster on the larger-payload IN transfers (wLength=64, which carry actual response data) and are suppressed when a vendor control transaction with smaller-payload IN reads (no response payload, wLength=14–15) is interleaved between two heavy-payload exchanges. The exact xhci_hcd state being reset isn't understood; we treat the interleaved transaction as a behavioral workaround at the protocol layer rather than a known mechanism fix.

Secondary issues:

  • pushMailbox reported success as soon as the reply DATA stage went on the wire (HAL TX_COMPLETED), not when the host actually received it.
  • pushMailbox discarded the entry's result code, so a SYSTEM_ERROR_CANCELLED from the system path could not propagate to the test.
  • Test bodies inserted delay(3s) after every pushMailbox as a workaround for the missing host-receipt signal, which helped but was not the fix.

Solution

  • Add an explicit c:'A' ACK request that the runner sends after parsing each mailbox reply. This is what completes the entry on the device side AND interleaves an additional vendor transaction (whose IN reads carry no payload, so they're the kernel-bug-safe variety) between the readMailbox and getStatus exchanges, suppressing the phantom-EPIPE stalls observed without it.
  • Tag each MailboxEntry with a monotonic id; include it in the read reply and in the ACK request so completion is idempotent and can't match the wrong entry.
  • Drop readMailbox's TX_COMPLETED-driven completion callback. Completion now happens from the ack handler instead.
  • Hold the MailboxEntry::wait loop for ACK_DELAY_MS (100 ms) past the ACK timestamp before returning, so the host's ACK response cycle finishes on the wire before the test continues into System.sleep and detaches USB.
  • Have pushMailbox return m->result() so a cancellation propagates to the test instead of being reported as success.
  • Bump every pushMailbox(..., 10000) site in sleep20_device.cpp to 20000 ms for headroom under retry.
  • Remove the delay(3s) workaround that followed every pushMailbox call in sleep20_device.cpp; the ACK mechanism replaces what the delay was guarding against.

Testing

  • Four consecutive runs of wiring/sleep20 on the affected rig: 46/46 tests passing, zero application-level USB stalls observed in usbmon captures.

References

  • Branch: fix/test-runner-mailbox-usb-stall (in device-os)
  • Companion PR: device-os-test-runner#44 on fix/mailbox-usb-stall

// many ms past the ACK timestamp before unblocking the test thread.
// This gives the host's ACK response cycle time to complete on the
// wire before the test continues into System.sleep and tears down USB.
static constexpr system_tick_t ACK_DELAY_MS = 100;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be needed I think. If we got the ACK the other side 100% got the message, right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was absolutely needed... and to some extent I can't see a way around having a tiny forced delay on the device side that waits for INIT-CHECK-END vendor request protocol to complete and the host to be in a good state before the device returns from the pushMailbox call and proceeds to System.sleep calls that can pull the rug out from under the host.

  • The host essentially sends the ACK INIT: OUT control transfer
  • Host starts checking for CHECK: IN control transfer of the ACK response.
  • Device receives the ACK, complete_'s and sends ACK response... but crucially waits up to 100ms so host has time to receive the ACK response.
  • Host finalizes transaction with END: control OUT to device.

It might seem like what is the point if the ACK has it's own "ack" that we need to busy wait for... but something about the ACK INIT-CHECK-END resolves the kernel driver bug just by changing the shape of the transfers. It might be because the ACK response is empty, header only. Vs. the readMailbox and getStatus transfers that request data with a 64 byte buffer and generally have a larger payload.

@technobly
technobly force-pushed the fix/test-runner-mailbox-usb-stall branch from 1b2e621 to d9928b7 Compare May 27, 2026 19:49
@technobly
technobly merged commit b1bad19 into develop May 27, 2026
14 checks passed
@technobly
technobly deleted the fix/test-runner-mailbox-usb-stall branch May 27, 2026 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants