fix(host/windows): two virtual pads stop tearing each other's reports #39

Merged

enricobuehler merged 1 commits from worktree-haptics-m7-windows into main 2026-08-04 21:03:17 +00:00

Conversation 0 Commits 1 Files Changed 3

+121 -34

enricobuehler commented 2026-08-04 17:30:05 +00:00

Owner

Copy Link

Copy Source

Workstream M7 of the haptics sweep — Windows drivers & inject. Closes B8, B9, R2.

B8 — the output ring could hand the host a torn report

Publishing is a read-modify-write — read the cursor, write the slot it names, advance it — and the framework dispatches output callbacks in parallel. Two could be inside it at once: both read the same head, both wrote the same slot, and both stored head + 1. The cursor moved once for two reports, and the host read a single entry with two reports mixed into it.

An atomic fetch_add does not fix this (the finding suggested a mutex, and it's right). fetch_add hands each writer its own slot but advances the cursor before the bytes exist — trading a torn slot for a torn slot the host is actively invited to read. Serializing the publish is what makes the cursor bump mean "the slot below is complete."

This matters because the ring exists specifically to stop a rumble STOP being coalesced away by a following LED/trigger report. A torn slot can eat that STOP, and there is no idle watchdog behind this path.

B9 — a promised memory ordering that never existed

Both drivers bumped their sequence counters with plain writes, while the host loads them with Acquire and says so in its own comments:

// Acquire pairs with the driver's publish-then-bump store order

An Acquire load pairs with a Release store and nothing else. As plain writes, these promised the host an ordering they never established — on a weakly-ordered core (ARM64) a fresh seq could arrive ahead of the bytes it announces. Both out_seq and rumble_seq are now Release.

pf-xusb's rumble seq was also racy the same way as the ring: two SET_STATE calls could both read one value and both write back value + 1, so the host — which treats an unchanged seq as "nothing new" — saw one bump for two writes and skipped a level. A skipped stop is the one that hurts: the pad buzzes until the ~2.5 s idle force-off notices the game went quiet, which is what bounds the damage.

Both locks are deliberately poison-tolerant. Poison is sticky, so this repo's usual if let Ok(g) = lock() idiom would silently stop publishing for the rest of the process after one unrelated panic. The protected state is bytes in a shared section, not an invariant a panic could have broken.

R2 — diagnosing a bad session made the session worse

The pad service thread — the one feeding input and rumble — waited up to two seconds for a pnputil enumeration, per unattached pad (the wait is a deadline, not a one-off: while the enumeration is outstanding, every pad pays it again), at exactly the moment a session was already going wrong.

The diagnosis now runs on its own thread. Off the hot path the wait stops having to be a compromise, so it is generous enough to report what it actually found — given pnputil routinely takes longer than the old two-second budget, the old code usually gave up and printed "still enumerating", the one answer that helps nobody.

---

Verification — on real Windows (192.168.1.133)

Drivers workspace (B8, B9), after cargo clean -p pf-gamepad -p pf-xusb:

• cargo clippy -p pf-umdf-util -p pf-xusb -p pf-gamepad -p pf-mouse -p wdk-iddcx -p pf-vdisplay --all-targets -- -D warnings — RC=0
• cargo fmt -p pf-umdf-util -p pf-xusb -p pf-gamepad -p pf-mouse --check — RC=0
• Confirmed both crates genuinely rebuilt (Compiling pf-xusb, Compiling pf-gamepad), not reused

pf-inject (R2), after cargo clean -p pf-inject:

• cargo clippy -p pf-inject --all-targets --locked -- -D warnings — RC=0
• cargo test -p pf-inject --locked — 69 passed / 0 failed
• Confirmed pf-inject genuinely rebuilt

Not verified

No behavioural proof on hardware. B8 and B9 are races — reproducing them wants two pads driven hard enough to overlap callbacks, and their absence can't be shown by a test run. What is verified is that the code compiles clean under -D warnings on the real toolchain and the existing suite still passes. R2's payoff (input not stalling while a driver fails to attach) needs a session with a deliberately-broken driver install.

Note for the next person

wdk-build's build script locates the workspace by walking OUT_DIR's ancestors for a Cargo.lock, so an out-of-tree CARGO_TARGET_DIR makes it panic with "a Cargo.lock file should exist in the same directory as the top-level Cargo.toml" — when the file is present and simply not on that path. The drivers workspace must build with its target dir in-tree. Cost one round-trip; recorded in the planning doc.

Workstream **M7** of the haptics sweep — Windows drivers & inject. Closes **B8, B9, R2**. ### B8 — the output ring could hand the host a torn report Publishing is a read-modify-write — read the cursor, write the slot it names, advance it — and the framework dispatches output callbacks in **parallel**. Two could be inside it at once: both read the same `head`, both wrote the **same slot**, and both stored `head + 1`. The cursor moved once for two reports, and the host read a single entry with two reports mixed into it. **An atomic `fetch_add` does not fix this** (the finding suggested a mutex, and it's right). `fetch_add` hands each writer its own slot but advances the cursor *before* the bytes exist — trading a torn slot for a torn slot the host is actively invited to read. Serializing the publish is what makes the cursor bump mean "the slot below is complete." This matters because the ring exists specifically to stop a rumble **STOP** being coalesced away by a following LED/trigger report. A torn slot can eat that STOP, and there is no idle watchdog behind this path. ### B9 — a promised memory ordering that never existed Both drivers bumped their sequence counters with **plain writes**, while the host loads them with `Acquire` and says so in its own comments: > `// Acquire pairs with the driver's publish-then-bump store order` An `Acquire` load pairs with a `Release` store and nothing else. As plain writes, these promised the host an ordering they never established — on a weakly-ordered core (ARM64) a fresh seq could arrive ahead of the bytes it announces. Both `out_seq` and `rumble_seq` are now `Release`. `pf-xusb`'s rumble seq was also racy the same way as the ring: two `SET_STATE` calls could both read one value and both write back `value + 1`, so the host — which treats an unchanged seq as "nothing new" — saw one bump for two writes and skipped a level. A skipped **stop** is the one that hurts: the pad buzzes until the ~2.5 s idle force-off notices the game went quiet, which is what bounds the damage. Both locks are deliberately **poison-tolerant**. Poison is sticky, so this repo's usual `if let Ok(g) = lock()` idiom would silently stop publishing for the rest of the process after one unrelated panic. The protected state is bytes in a shared section, not an invariant a panic could have broken. ### R2 — diagnosing a bad session made the session worse The pad service thread — the one feeding input and rumble — waited up to two seconds for a `pnputil` enumeration, **per unattached pad** (the wait is a deadline, not a one-off: while the enumeration is outstanding, every pad pays it again), at exactly the moment a session was already going wrong. The diagnosis now runs on its own thread. Off the hot path the wait stops having to be a compromise, so it is generous enough to report what it actually found — given pnputil routinely takes longer than the old two-second budget, the old code usually gave up and printed "still enumerating", the one answer that helps nobody. --- ### Verification — on real Windows (`192.168.1.133`) Drivers workspace (B8, B9), after `cargo clean -p pf-gamepad -p pf-xusb`: - `cargo clippy -p pf-umdf-util -p pf-xusb -p pf-gamepad -p pf-mouse -p wdk-iddcx -p pf-vdisplay --all-targets -- -D warnings` — **RC=0** - `cargo fmt -p pf-umdf-util -p pf-xusb -p pf-gamepad -p pf-mouse --check` — **RC=0** - Confirmed both crates genuinely rebuilt (`Compiling pf-xusb`, `Compiling pf-gamepad`), not reused `pf-inject` (R2), after `cargo clean -p pf-inject`: - `cargo clippy -p pf-inject --all-targets --locked -- -D warnings` — **RC=0** - `cargo test -p pf-inject --locked` — **69 passed / 0 failed** - Confirmed `pf-inject` genuinely rebuilt ### Not verified No behavioural proof on hardware. B8 and B9 are races — reproducing them wants two pads driven hard enough to overlap callbacks, and their absence can't be shown by a test run. What is verified is that the code compiles clean under `-D warnings` on the real toolchain and the existing suite still passes. R2's payoff (input not stalling while a driver fails to attach) needs a session with a deliberately-broken driver install. ### Note for the next person `wdk-build`'s build script locates the workspace by walking **`OUT_DIR`'s ancestors** for a `Cargo.lock`, so an out-of-tree `CARGO_TARGET_DIR` makes it panic with *"a Cargo.lock file should exist in the same directory as the top-level Cargo.toml"* — when the file is present and simply not on that path. The drivers workspace must build with its target dir in-tree. Cost one round-trip; recorded in the planning doc.

enricobuehler added 1 commit 2026-08-04 17:30:07 +00:00

fix(host/windows): two virtual pads stop tearing each other's reports ... 31b5f90b12

Three faults on the Windows pad path, two of them races that only bite when a
game drives a pad hard enough for two callbacks to overlap.

pf-gamepad's output ring could hand the host a torn report. Publishing is a
read-modify-write — read the cursor, write the slot it names, advance it — and
the framework dispatches output callbacks in parallel, so two could be inside
it at once: both read the same head, both wrote the SAME slot, and both stored
head+1, so the cursor moved once for two reports and the host read a single
entry with two reports mixed into it. An atomic fetch_add does not fix this. It
hands each writer its own slot but advances the cursor before the bytes exist,
so the host is then invited to read a slot still being filled. Serializing the
publish is what makes the cursor bump mean "the slot below is complete". The
ring exists to stop a rumble STOP being coalesced away, and a torn slot can eat
that STOP with no idle watchdog behind it.

Both drivers also promised the host an ordering they never established. The
host loads out_seq and rumble_seq with Acquire and says so in its own comments
— "Acquire pairs with the driver's publish-then-bump store order" — but the
drivers bumped both with plain writes, and an Acquire load pairs with a Release
store and nothing else. On a weakly-ordered core the host could see a fresh seq
against stale bytes. pf-xusb's rumble seq was racy in the same way as the ring:
two SET_STATE calls could both read one value and both write back value+1, so
the host saw one bump for two writes and skipped a level. A skipped stop is the
one that hurts — the pad buzzes until the ~2.5 s idle force-off notices the
game went quiet, which is what bounds the damage.

Diagnosing an unattached driver stalled the session. The pad service thread —
the one feeding input and rumble — waited up to two seconds for a pnputil
enumeration, per unattached pad, at exactly the moment a session was already
going wrong. The diagnosis now runs on its own thread. Off the hot path the
wait no longer has to be a compromise, so it is generous enough to report what
it actually found instead of giving up with "still enumerating" — which, given
pnputil routinely takes longer than the old budget, is what it usually did.

enricobuehler marked the pull request as ready for review 2026-08-04 21:02:47 +00:00

enricobuehler merged commit 956d8dd8ef into main 2026-08-04 21:03:17 +00:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

No items

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

FernandoMX enricobuehler (Enrico Bühler) kszlim nannku neoney sassycrown

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: unom/punktfunk#39