Skip to content

[bug fix] driver_vive: fix three USB transfer retry/error-handling bugs in handle_transfer - #377

Closed
rocketmark wants to merge 1 commit into
collabora:masterfrom
rocketmark:upstream-pr/driver-vive-retry-logic
Closed

rocketmark wants to merge 1 commit into
collabora:masterfrom
rocketmark:upstream-pr/driver-vive-retry-logic

Conversation

@rocketmark

Copy link
Copy Markdown
Contributor

Summary

handle_transfer()'s USB transfer retry/error-handling logic has three bugs: the timeout path never resubmits below the disconnect threshold, error_count is incremented twice per call, and the error path always disconnects regardless of retry outcome. A fourth, related bug in the same path: a STALL is retried without clearing the halt condition first.

Demonstration

Trigger: ordinary USB transfer errors (a timeout, a stall) during normal operation — not a rare or exotic condition.

BEFORE fix:
status = LIBUSB_TRANSFER_TIMED_OUT, consecutive_timeouts = 1 (below the disconnect threshold)
libusb_submit_transfer() never called
Result: endpoint stops producing further completions, permanently

status = LIBUSB_TRANSFER_STALL
error_count++ → 1
error_count++ (again, inside the if condition) → 2
libusb_submit_transfer() called, resubmit succeeds
no return — falls through to goto disconnect anyway
libusb_clear_halt() never called
Result: device disconnects after a single transient STALL; error_count
reaches the threshold in half the intended number of errors

AFTER fix:
status = LIBUSB_TRANSFER_TIMED_OUT, consecutive_timeouts = 1
libusb_submit_transfer() called
Result: endpoint resubmitted, keeps operating

status = LIBUSB_TRANSFER_STALL
libusb_clear_halt() called
error_count++ → 1 (single increment)
libusb_submit_transfer() called, resubmit succeeds, return
Result: device retries up to the intended 10 consecutive errors before
disconnecting, clearing the halt condition first each time
Impact

Reachable any time a USB transfer doesn't complete cleanly — a timeout, a stall, any transient bus error. This is the normal error-recovery path, not an edge case. As it stands, a single STALL disconnects the device outright instead of retrying, and any endpoint that times out even once stops producing further completions permanently, since nothing ever resubmits it.

Change

One file, src/driver_vive.libusb.h. Three changes in handle_transfer(): (1) resubmit the transfer before returning on a sub-threshold timeout instead of just returning, (2) a single error_count++ instead of two, with a return after a successful resubmit instead of always falling through to goto disconnect, (3) call libusb_clear_halt() before retrying a STALL. Includes a 3-case regression test (driver_vive_retry_test.c) against a fake libusb (mock_libusb.c), reproducing each bug independently and without hardware.

Found via

Found while auditing handle_transfer()'s retry/error-handling logic for a related reentrancy issue in the same function (see the companion PR). Confirmed each of the three bugs independently with a deterministic test harness rather than relying on a specific field observation.

…le_transfer

Three distinct bugs in handle_transfer()'s retry/error-handling logic,
all confirmed with a new regression test (driver_vive_retry_test.c) that
drives the real, unmodified handle_transfer/AttachInterface code against
a fake libusb (mock_libusb.c, no hardware needed):

1. On LIBUSB_TRANSFER_TIMED_OUT, below the consecutive-timeout/RF-device
   disconnect threshold, the function returned without resubmitting the
   transfer. Once an endpoint times out even once, nothing resubmits it
   again, so it silently stops producing further transfer completions.

2. error_count was incremented twice per call (`error_count++` then
   `if (error_count++ < 10)`), so the disconnect threshold was reached in
   half the intended number of consecutive errors.

3. After a successful resubmit in the error-handling path, execution fell
   through to `goto disconnect` regardless of the retry outcome -- so a
   single transient error (one STALL, for example) disconnected the
   device immediately instead of retrying up to the intended threshold.

A fourth, related fix in the same path: LIBUSB_TRANSFER_STALL was retried
without calling libusb_clear_halt() first, so the retry was guaranteed to
stall again.

Before this fix, with the included test:
  consecutive_timeouts: 1, submit calls: 0           (never resubmitted)
  error_count: 2 (expected 1), shutdown: 1 (expected 0), clear_halt
    calls: 0 (expected 1)                             (double increment,
                                                        disconnects on the
                                                        first error, no
                                                        clear_halt)
After: all three pass with the expected values.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@rocketmark

Copy link
Copy Markdown
Contributor Author

Closing — two of the three fixes here (error_count double-increment, missing return after a successful resubmit) overlap with #372, which found the same missing-return bug via an actual observed crash (libusb_free_transfer() on a still-in-flight transfer). I'll resubmit a trimmed PR with just the two fixes #372 doesn't cover: the timeout path not resubmitting below the disconnect threshold, and the missing libusb_clear_halt() before retrying a STALL.

@rocketmark rocketmark closed this Oct 10, 2026
@rocketmark
rocketmark deleted the upstream-pr/driver-vive-retry-logic branch October 10, 2026 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant