From 31b5f90b129ac5cd13602e639e61a52d166066aa Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Tue, 4 Aug 2026 19:20:01 +0200 Subject: [PATCH] fix(host/windows): two virtual pads stop tearing each other's reports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../src/inject/windows/gamepad_raii.rs | 95 ++++++++++++------- .../windows/drivers/pf-gamepad/src/lib.rs | 30 +++++- packaging/windows/drivers/pf-xusb/src/lib.rs | 30 +++++- 3 files changed, 121 insertions(+), 34 deletions(-) diff --git a/crates/pf-inject/src/inject/windows/gamepad_raii.rs b/crates/pf-inject/src/inject/windows/gamepad_raii.rs index b4679169..dded23fc 100644 --- a/crates/pf-inject/src/inject/windows/gamepad_raii.rs +++ b/crates/pf-inject/src/inject/windows/gamepad_raii.rs @@ -819,46 +819,77 @@ impl DriverAttach { /// One-shot WARN with everything the host can find out about WHY the driver isn't attached: /// driver-store presence, the devnode's PnP status/problem code, and where to look next. + /// + /// Runs on its own thread and returns immediately. The caller is the session's pad service + /// thread — the one feeding input and rumble — and everything below is slow: the driver-store + /// check waits up to [`INVENTORY_WAIT`] for a `pnputil` enumeration that can take tens of + /// seconds, and the devnode lookup is a synchronous PnP call. Blocking there stalled input for + /// up to two seconds *per unattached pad* (the wait is a deadline, not a one-off: while the + /// enumeration is still outstanding every pad pays it again), at exactly the moment a session + /// is already going wrong. Diagnostics must never be able to hurt the thing they diagnose. + /// + /// Off the hot path the wait also stops being a compromise — it can afford to be patient and + /// report what it actually found rather than "still enumerating". fn diagnose(&self) { - let store = match driver_store_has(self.inf) { - Some(true) => "driver package present in the driver store", - Some(false) => { - "driver package NOT in the driver store — run: punktfunk-host.exe driver install --gamepad" - } - None => "driver store could not be queried (pnputil failed or still enumerating)", - }; - let devnode = match &self.instance_id { - Some(id) => devnode_status_line(id), - None => { - "no per-session devnode (SwDeviceCreate failed earlier — see the warning above)" - .to_string() - } - }; - tracing::warn!( - driver = self.driver, - shm = %self.shm_name, - grace_secs = ATTACH_GRACE.as_secs(), - store, - devnode = %devnode, - driver_log = self.driver_log, - "gamepad driver has not attached to the shared section — the virtual pad exists but no \ - driver is serving it (games will not see it); an old (pre-sealed-channel) driver also \ - reads as not-attached: update with punktfunk-host.exe driver install --gamepad \ - (driver_log is only written by debug driver builds, or with the PFXUSB_DEBUG_LOG / \ - PFGAMEPAD_DEBUG_LOG / PFMOUSE_DEBUG_LOG system env var set + the device restarted)" - ); + let (driver, inf, driver_log) = (self.driver, self.inf, self.driver_log); + let shm_name = self.shm_name.clone(); + let instance_id = self.instance_id.clone(); + std::thread::Builder::new() + .name("pf-driver-diagnose".into()) + .spawn(move || diagnose_blocking(driver, inf, driver_log, &shm_name, instance_id)) + .ok(); } } -/// How long [`driver_store_inventory`] lets the caller wait for the background pnputil query -/// before reporting without it — [`observe`] runs on the pad service thread, which must keep -/// draining pad slots even when the driver store is wedged. -const INVENTORY_WAIT: Duration = Duration::from_secs(2); +/// The body of [`DriverAttach::diagnose`], on its own thread. Split out rather than inlined into +/// the closure so the blocking calls stay visible as blocking. +fn diagnose_blocking( + driver: &'static str, + inf: &'static str, + driver_log: &'static str, + shm_name: &str, + instance_id: Option, +) { + let store = match driver_store_has(inf) { + Some(true) => "driver package present in the driver store", + Some(false) => { + "driver package NOT in the driver store — run: punktfunk-host.exe driver install --gamepad" + } + None => "driver store could not be queried (pnputil failed or still enumerating)", + }; + let devnode = match &instance_id { + Some(id) => devnode_status_line(id), + None => "no per-session devnode (SwDeviceCreate failed earlier — see the warning above)" + .to_string(), + }; + tracing::warn!( + driver, + shm = %shm_name, + grace_secs = ATTACH_GRACE.as_secs(), + store, + devnode = %devnode, + driver_log, + "gamepad driver has not attached to the shared section — the virtual pad exists but no \ + driver is serving it (games will not see it); an old (pre-sealed-channel) driver also \ + reads as not-attached: update with punktfunk-host.exe driver install --gamepad \ + (driver_log is only written by debug driver builds, or with the PFXUSB_DEBUG_LOG / \ + PFGAMEPAD_DEBUG_LOG / PFMOUSE_DEBUG_LOG system env var set + the device restarted)" + ); +} + +/// How long [`driver_store_inventory`] waits for the background pnputil query before reporting +/// without it. Only [`diagnose_blocking`] waits, and that has a thread to itself, so this is +/// generous: pnputil routinely takes longer than a couple of seconds on a busy driver store, and +/// the old two-second budget — chosen to limit the damage while this ran on the pad service thread +/// — meant the diagnosis usually gave up and printed "still enumerating", which is the one answer +/// that helps nobody. Nothing waits on this thread, so patience costs only a late log line. +const INVENTORY_WAIT: Duration = Duration::from_secs(30); /// Driver-store inventory (`pnputil /enum-drivers`), lower-cased, fetched once per process — only /// consulted on the failure path, so the subprocess cost never hits a healthy session. The query /// runs on its OWN thread: pnputil can block for tens of seconds on a busy/wedged driver store, -/// and the caller is the pad service thread. `None` = not available yet (query still running) or +/// and this keeps one wedged query from being re-run per pad. `None` = not available yet (query +/// still running past [`INVENTORY_WAIT`]) or /// failed; a query that outlives [`INVENTORY_WAIT`] still lands in the cache for later reports. fn driver_store_inventory() -> Option<&'static str> { static INV: OnceLock = OnceLock::new(); diff --git a/packaging/windows/drivers/pf-gamepad/src/lib.rs b/packaging/windows/drivers/pf-gamepad/src/lib.rs index daefbcb8..96fb6d5e 100644 --- a/packaging/windows/drivers/pf-gamepad/src/lib.rs +++ b/packaging/windows/drivers/pf-gamepad/src/lib.rs @@ -360,9 +360,32 @@ fn ring_len(view: &pf_umdf_util::section::MappedView) -> u32 { /// from being coalesced away by a following LED/trigger report inside one host poll window (the /// confirmed stuck-rumble path). fn publish_output(view: &pf_umdf_util::section::MappedView, bytes: &[u8]) { + // Serialized: the whole publish is a read-modify-write (read the cursor, write the slot it + // names, then advance it) and the framework dispatches output callbacks in PARALLEL, so two + // can be inside this at once. Unsynchronized, both read the same `ring_head`, both write the + // SAME slot — tearing one report's bytes across the other's — and both store head+1, so the + // cursor advances once for two reports and the host sees a single torn entry. + // + // An atomic `fetch_add` on the head does not fix it. That hands each writer a distinct slot, + // but it advances the cursor BEFORE the slot bytes exist, so the host can read a slot that is + // still being filled — trading a torn slot for a torn slot the host is invited to read. Making + // the head-advance mean "the slot below is complete" is exactly what the lock buys. + // + // Poison-tolerant on purpose. Poison is sticky, so the repo's usual `if let Ok(g) = lock()` + // would skip the publish for the REST OF THE PROCESS after a single panic elsewhere — silently + // ending game output. Recovering the guard is safe here: the protected state is bytes in a + // shared section, not an invariant a panic could have broken. + let _publish = RING_PUBLISH + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); view.write_bytes(OFF_OUTPUT, bytes); let seq = view.read_u32(OFF_OUT_SEQ).wrapping_add(1); - view.write_u32(OFF_OUT_SEQ, seq); + // Release, not a plain write: the host loads `out_seq` with Acquire specifically to order its + // copy of the report bytes after it (`dualsense_windows.rs`, "Acquire pairs with the driver's + // publish-then-bump store order"). An Acquire load pairs with a Release store and nothing + // else, so as a plain write this promised the host an ordering it never actually established — + // on a weakly-ordered core (ARM64) the fresh seq could arrive ahead of the bytes it announces. + view.store_u32(OFF_OUT_SEQ, seq, Ordering::Release); let len = ring_len(view); if len != 0 { let head = view.read_u32(OFF_RING_HEAD); @@ -375,6 +398,11 @@ fn publish_output(view: &pf_umdf_util::section::MappedView, bytes: &[u8]) { } } +/// Serializes [`publish_output`] against itself — see the note there for why an atomic cursor is +/// not enough. Uncontended in the common case: one output report at a time is the norm, and the +/// critical section is a few dozen bytes of memcpy into an already-mapped view. +static RING_PUBLISH: std::sync::Mutex<()> = std::sync::Mutex::new(()); + /// The sealed-channel client (per-pad: `ProcessSharingDisabled` gives each pad its own WUDFHost, so /// this static is per-pad). The handshake/adoption/validation state machine lives in `pf_umdf_util`. static CHANNEL: ChannelClient = ChannelClient::new(); diff --git a/packaging/windows/drivers/pf-xusb/src/lib.rs b/packaging/windows/drivers/pf-xusb/src/lib.rs index 58ccdf64..5343a8fb 100644 --- a/packaging/windows/drivers/pf-xusb/src/lib.rs +++ b/packaging/windows/drivers/pf-xusb/src/lib.rs @@ -358,20 +358,48 @@ fn read_state(data: Option<&MappedView>) -> (u32, u16, u8, u8, i16, i16, i16, i1 /// host can tell "driver bound and alive" apart from "driver package missing/failed to bind" and see /// the game-visible polling path advance. fn touch_driver_marks(data: &MappedView) { + let _marks = SECTION_PUBLISH + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); data.write_u32(OFF_DRIVER_PROTO, GAMEPAD_PROTO_VERSION); let hb = data.read_u32(OFF_DRIVER_HEARTBEAT).wrapping_add(1); data.write_u32(OFF_DRIVER_HEARTBEAT, hb); } /// Publish a game's rumble (from SET_STATE) into the DATA section for the host to forward. +/// +/// Serialized and Release-published, because IOCTLs arrive concurrently and neither property held +/// before. `seq` was a read-modify-write across the two motor bytes: two `SET_STATE` calls could +/// both read the same value and both write back `seq + 1`, so the host — which treats an unchanged +/// seq as "nothing new" — saw one bump for two writes and skipped a level entirely. A skipped +/// **stop** is the one that hurts: the pad keeps buzzing until the host's ~2.5 s idle force-off +/// notices the game went quiet, which is where the bound on this bug comes from. +/// +/// The seq store is Release for the same reason as `pf-gamepad`'s `out_seq`: the host loads it with +/// Acquire and documents that as ordering its read of the motor bytes ("the driver bumps +/// `rumble_seq` AFTER writing the rumble bytes", `gamepad_windows.rs`). A plain write gives that +/// Acquire nothing to pair with, so the guarantee the host's comment claims did not exist in either +/// direction — the host could read a fresh seq against stale motor levels on a weakly-ordered core. fn publish_rumble(data: Option<&MappedView>, large: u8, small: u8) { let Some(v) = data else { return }; + let _publish = SECTION_PUBLISH + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); v.write_u8(OFF_RUMBLE_LARGE, large); v.write_u8(OFF_RUMBLE_SMALL, small); let seq = v.read_u32(OFF_RUMBLE_SEQ).wrapping_add(1); - v.write_u32(OFF_RUMBLE_SEQ, seq); + v.store_u32(OFF_RUMBLE_SEQ, seq, Ordering::Release); } +/// Serializes the section's read-modify-write publishes ([`publish_rumble`], [`touch_driver_marks`]) +/// against each other. One lock rather than one per field: they are all short byte writes into the +/// same mapped view, and the contention is nil compared to the IOCTL round trip that reaches them. +/// +/// Poison-tolerant deliberately — poison is sticky, so bailing out on it would silently stop +/// forwarding rumble for the rest of the process. The protected state is bytes in a shared section, +/// not an invariant a panic elsewhere could have violated. +static SECTION_PUBLISH: std::sync::Mutex<()> = std::sync::Mutex::new(()); + // Build the 29-byte GET_STATE buffer (the layout xinput1_4 parses). fn build_get_state(data: Option<&MappedView>) -> [u8; 29] { let (packet, buttons, lt, rt, lx, ly, rx, ry) = read_state(data); -- 2.54.0