Skip to content

fix(vader5): claim IF1 via libusb so the physical pad has no hidraw node#357

Merged
BANANASJIM merged 12 commits into
mainfrom
fix/vader5-libusb-claim-355
Jun 2, 2026
Merged

fix(vader5): claim IF1 via libusb so the physical pad has no hidraw node#357
BANANASJIM merged 12 commits into
mainfrom
fix/vader5-libusb-claim-355

Conversation

@BANANASJIM

@BANANASJIM BANANASJIM commented May 31, 2026

Copy link
Copy Markdown
Owner

What changed

The pad exposes three HID interfaces (IF1 vendor 0xffa0, IF2 Mouse, IF3 vendor 0xffee), each producing its own /dev/hidrawN node via hid-generic. Claiming only IF1 left IF2/IF3 nodes visible, so Steam still saw the raw pad. This PR claims all three so the kernel exposes no hidraw/evdev node for the physical pad.

  • devices/flydigi/vader5.toml
    • IF1: class = "vendor", ep_in = 0x82, ep_out = 0x06 — read for input + rumble + init (as before).
    • IF2 (id = 2) and IF3 (id = 3): new class = "suppress" blocks — claimed only to evict hid-generic; never read or written.
    • block_kernel_drivers extended to ["xpad", "hid_generic", "usbhid"] (proactive pre-unbind), scoped to 37d7:2401 by the udev rule.
  • New "suppress" interface class (src/config/device.zig)
    • validate() accepts hid / vendor / suppress; a suppress interface must have no ep_in/ep_out and must not be referenced by any report.interface, commands.*.interface, or init.interface.
  • UsbrawSuppress (src/io/usbraw.zig)
    • Minimal libusb claim path: same libusb_init / open_device_with_vid_pid / detach_kernel_driver / claim_interface sequence as UsbrawDevice.open, but spawns no pipe and no read thread. Exposes only openSuppress/close; it is not a DeviceIO and consumes no poll slot.
  • Two-pass device open (src/device_instance.zig)
    • devices[] is sized to the count of non-suppress interfaces. Pass 1 opens hid/vendor interfaces into devices[]; Pass 2 claims suppress interfaces into a separate suppress_devs[]. Any claim failure (e.g. IF3 busy) is fail-closed — the error propagates and everything already claimed is released. deinit releases each suppress claim and frees the slice.
  • Order-independent routing (src/config/device.zig, src/event_loop.zig, src/supervisor.zig)
    • openedInterfaceCount / deviceIndexForInterface / interfaceForDeviceIndex map between a USB interface id and the devices[] index counting only non-suppress interfaces, so resolveIfaceIdx, the read-loop interface lookup, init, re-init, and the hotplug rebind path all stay correct regardless of TOML interface ordering. Suppress interfaces stay claimed across suspend/resume. MAX_FDS/device_base are unchanged.

Why

On recent Steam/Bazzite the physical Flydigi Vader 5 (37d7:2401) stays visible as a generic Xbox pad because Steam reads /dev/hidrawN directly, bypassing padctl's evdev EVIOCGRAB. Each of the pad's three HID interfaces gets its own hidraw node, so suppressing only IF1 is insufficient. Claiming every HID interface via libusb means the kernel exposes no hidraw/evdev node for the device — there is nothing for Steam to read — leaving only the padctl-emulated Xbox Elite + virtual mouse output. padctl emits its own virtual mouse and virtual Elite, so it needs neither IF2 nor IF3 data; they are claimed suppress-only.

Confirmed

  • Root cause — a Bazzite reporter's lsof /dev/hidraw* shows steam holding the physical pad's hidraw node directly (steam ... /dev/hidraw1). evtest also shows padctl's emulated Elite receives all buttons (incl. paddles) correctly, but Steam reads the raw device instead — so the same hidraw-direct read drives both the duplicate pad and the missing-paddles symptom.
  • Endpoints / interface layout — confirmed on a real Vader 5 via lsusb -v -d 37d7:2401: IF1 (HID) EP 0x82 IN / 0x06 OUT; IF2 (HID) Mouse; IF3 (HID) vendor 0xffee; three hid-generic hidraw nodes exist while padctl runs.

Tests

  • New Layer-0 tests (src/config/device.zig):
    • A mixed report + suppress config parses; suppress interfaces are excluded from the opened-devices count; deviceIndexForInterface/interfaceForDeviceIndex resolve the report interface to the correct devices[] index whether the suppress interface precedes or follows it.
    • validate() rejects a suppress interface that declares endpoints, and rejects a report referencing a suppress interface.
  • Updated the vader5 vendor-transport test: interface count 1 → 3, asserting IF1 stays vendor with 0x82/0x06 and IF2/IF3 are suppress; openedInterfaceCount == 1.
  • Integration test driving DeviceInstance.init with a suppress interface preceding the report interface, asserting the init handshake routes to the helper-mapped device, not a positional interface[i] ↔ devices[i] index. Proven falsifiable: injecting the positional regression makes exactly this test fail.

Test plan

  • ./scripts/padctl-docker build: exit 0.
  • ./scripts/padctl-docker test: exit 0 (includes the new suppress/index tests).
  • Docker test-tsan: exit 0.

Real-hardware validation checklist (pre-merge gate)

CI/build-green is not sufficient — claiming IF2/IF3 must be confirmed not to break IF1 input on the physical device before merge.

  • lsusb -v -d 37d7:2401 — IF1 EP 0x82 IN / 0x06 OUT, IF2 Mouse, IF3 vendor 0xffee confirmed.
  • With padctl stopped, note the Flydigi hidraw nodes (ls -l /dev/hidraw* | <map to 37d7:2401> or udevadm).
  • Start padctl, then confirm zero Flydigi (37d7:2401) hidraw nodes remain while padctl runs (all three HID interfaces claimed by libusb).
  • Confirm the main gamepad input still works (sticks, buttons, triggers, gyro).
  • Confirm the generic Xbox pad no longer appears as a second controller in Steam.
  • Confirm rumble still works through the libusb transport.
  • Confirm a suspend/resume (sleep/wake) cycle keeps IF2/IF3 suppressed and re-binds IF1 input correctly.

Refs #355

Summary by CodeRabbit

  • New Features

    • Support for suppressed (non-readable) USB interfaces and periodic liveness sweeps for libusb-backed devices.
  • Bug Fixes

    • Init/rerun now target the correct readable interface when suppressed interfaces are present.
    • Better kernel-driver reattach/cleanup on open/close and multi-interface exposure handling for Flydigi Vader 5 Pro.
  • Improvements

    • More robust suspend/resume, teardown, and device indexing across mixed-interface devices.
  • Tests

    • New fixtures and tests for suppressed-interface layouts, init routing, disconnect handling, and liveness teardown.

…ode (#355)

Switch Vader 5 IF1 from class "hid" to class "vendor" so padctl opens the
interface through libusb (detach kernel driver + claim interface). With the
interface claimed there is no kernel hidraw/evdev node, so Steam can no
longer read the physical pad directly and surface it as a generic Xbox
controller alongside the padctl-emulated output.

Init handshake and rumble both go through the DeviceIO.write vtable, so the
libusb transport runs them identically to the previous hidraw path. Extend
block_kernel_drivers so the VID:PID-scoped proactive udev unbind also covers
hid_generic/usbhid before padctl claims the interface.

Adds a config-parse regression test locking IF1 to the vendor transport with
ep_in=0x82 / ep_out=0x06.
@coderabbitai

coderabbitai Bot commented May 31, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 700004b6-33b9-4d41-89ac-89e10b8181a0

📥 Commits

Reviewing files that changed from the base of the PR and between 1f8a934 and 9c8be4b.

📒 Files selected for processing (1)
  • src/supervisor.zig
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/supervisor.zig

📝 Walkthrough

Walkthrough

Adds support for “suppress” USB interfaces: config helpers/validation, a UsbrawSuppress type to claim interfaces, DeviceInstance tracking and routing that skips suppress interfaces, event-loop/supervisor mapping and liveness teardown, plus tests and a Vader5 TOML fixture.

Changes

Suppress USB interface support

Layer / File(s) Summary
Config helpers, validation, and tests
src/config/device.zig, devices/flydigi/vader5.toml, tests
Adds isSuppressClass, openedInterfaceCount, deviceIndexForInterface, interfaceForDeviceIndex; extends validate() with suppress pre-pass; adds tests for Vader5 TOML, ordering-independence, and invalid suppress usages.
DeviceInstance init, suppress tracking, and tests
src/device_instance.zig, tests
Track suppress_devs, allocate devices by openedInterfaceCount, open suppress interfaces separately, route device.init handshakes via deviceIndexForInterface, store and close suppress handles, update rerunInitSequence, and add regression tests.
Usbraw cleanup and UsbrawSuppress
src/io/usbraw.zig, compat/libusb-1.0/libusb.h
Factor disconnect/close helpers; add UsbrawSuppress with openSuppress/close to claim/release interfaces and re-attach kernel drivers; add compat stub for libusb_attach_kernel_driver; add unit test for signalDisconnect.
Event loop interface indexing
src/event_loop.zig
Import device_cfg and use deviceIndexForInterface / interfaceForDeviceIndex for runtime interface resolution so suppress interfaces are skipped and TOML order does not affect device-slot mapping.
Supervisor liveness sweep and rebind changes
src/supervisor.zig, tests
Add 1s liveness timer fd and sweepLivenessLibusb() to tear down libusb-holding instances; add instanceHoldsLibusb(); avoid suspending libusb-holding instances on detach; size/skip suppress interfaces during rebind; add fixtures and tests.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • BANANASJIM/padctl#114: Introduces rerunInitSequence for device rebind; this PR updates that function to route init handshakes through suppress-aware interface indexing.

Poem

🐰 A rabbit’s ode to claimed endpoints:
I hop where kernel drivers once would cling,
Suppress interfaces hush the noisy ring,
One vendor speaks while two stand guard and wait,
Init handshakes find their proper device gate,
Quieted drivers, and tests that prove it so.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title directly addresses the main change: claiming a Flydigi Vader 5 interface via libusb to suppress hidraw node exposure, which is the primary objective of this changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/vader5-libusb-claim-355

Warning

Review ran into problems

🔥 Problems

Stopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a @coderabbit review after the pipeline has finished.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

BANANASJIM added 4 commits May 30, 2026 21:52
Add a minimal libusb claim path that detaches the kernel driver and
claims an interface without spawning a read thread or pipe. Used to
evict hid-generic from auxiliary HID interfaces so the kernel exposes
no hidraw node for them. Exposes only openSuppress/close; it is not a
DeviceIO and consumes no poll slot.
Add a "suppress" interface class: claimed via libusb solely to evict
the kernel driver so no hidraw node is exposed, never read or written.
validate() accepts hid/vendor/suppress, requires suppress interfaces to
have no endpoints, and rejects any report/command/init referencing one.

Split the device-open path into two passes: open hid/vendor interfaces
into devices[] and claim suppress interfaces into a separate
suppress_devs[] slice that consumes no poll slot. Any claim failure is
fail-closed. Interface-to-devices[] index mapping (openedInterfaceCount,
deviceIndexForInterface, interfaceForDeviceIndex) counts only
non-suppress interfaces so report/command routing and the hotplug rebind
path stay correct regardless of TOML interface ordering.
The pad exposes three HID interfaces (IF1 vendor 0xffa0, IF2 Mouse, IF3
vendor 0xffee), each producing a hidraw node via hid-generic. Claiming
only IF1 left IF2/IF3 nodes visible, so Steam still saw the raw pad.
Declare IF2/IF3 as suppress-class so padctl claims all three interfaces
and the kernel exposes no hidraw/evdev node for the physical pad.

Refs #355
@BANANASJIM BANANASJIM marked this pull request as ready for review May 31, 2026 06:42
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/device_instance.zig (1)

254-254: 💤 Low value

Consider adding an explicit suppressInterfaceCount helper.

The suppress_count is derived arithmetically as total - opened. While correct (validation ensures all interfaces are hid/vendor/suppress), an explicit device_cfg.suppressInterfaceCount(cfg) helper would make the intent clearer and be more robust to future changes.

♻️ Proposed change

In src/config/device.zig, add:

pub fn suppressInterfaceCount(cfg: *const DeviceConfig) usize {
    var n: usize = 0;
    for (cfg.device.interface) |iface| {
        if (isSuppressClass(iface.class)) n += 1;
    }
    return n;
}

Then in src/device_instance.zig line 254:

-        const suppress_count = cfg.device.interface.len - device_cfg.openedInterfaceCount(cfg);
+        const suppress_count = device_cfg.suppressInterfaceCount(cfg);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/device_instance.zig` at line 254, Replace the arithmetic computation of
suppress_count with a dedicated helper to make intent explicit and future-proof:
add a new pub fn suppressInterfaceCount(cfg: *const DeviceConfig) usize in the
DeviceConfig module (use a loop over cfg.device.interface and increment when
isSuppressClass(iface.class) is true), then in device_instance.zig replace the
expression const suppress_count = cfg.device.interface.len -
device_cfg.openedInterfaceCount(cfg); with a call to
device_cfg.suppressInterfaceCount(cfg) (or compute opened vs suppressed only via
openedInterfaceCount and the new suppressInterfaceCount as needed). Ensure the
helper is exported as device_cfg.suppressInterfaceCount and used in place of the
arithmetic subtraction.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/device_instance.zig`:
- Line 254: Replace the arithmetic computation of suppress_count with a
dedicated helper to make intent explicit and future-proof: add a new pub fn
suppressInterfaceCount(cfg: *const DeviceConfig) usize in the DeviceConfig
module (use a loop over cfg.device.interface and increment when
isSuppressClass(iface.class) is true), then in device_instance.zig replace the
expression const suppress_count = cfg.device.interface.len -
device_cfg.openedInterfaceCount(cfg); with a call to
device_cfg.suppressInterfaceCount(cfg) (or compute opened vs suppressed only via
openedInterfaceCount and the new suppressInterfaceCount as needed). Ensure the
helper is exported as device_cfg.suppressInterfaceCount and used in place of the
arithmetic subtraction.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2b8d8dba-92a8-4276-bb67-55c3eb731128

📥 Commits

Reviewing files that changed from the base of the PR and between acdbcee and 61c09e8.

📒 Files selected for processing (6)
  • devices/flydigi/vader5.toml
  • src/config/device.zig
  • src/device_instance.zig
  • src/event_loop.zig
  • src/io/usbraw.zig
  • src/supervisor.zig

@BANANASJIM BANANASJIM marked this pull request as draft May 31, 2026 06:50
openplan-implementer and others added 5 commits May 31, 2026 01:04
After libusb releases an interface the kernel does not re-probe its driver,
so the suppressed hidraw node never returns until physical replug. Best-effort
attach restores the device on graceful teardown (SIGTERM -> deinit). SIGKILL or
crash cannot be restored; that is a known limitation of this mechanism.
…draw REMOVE

Mechanism A's own libusb claim deletes the device's /dev/hidrawN, which the
hidraw-uevent-driven supervisor reads as a physical unplug: it suspends the
instance and tears down the emulated output after the grace window.

detach() now peeks the devname binding and returns early for libusb-backed
instances (suppress claim or vendor read interface), leaving the binding intact
so the instance stays addressable. Pure hid-class instances keep the existing
fetchRemove + suspend + grace path unchanged.

Real physical unplug for libusb instances is detected by a new recurring 1s
liveness sweep that probes the UsbrawDevice pipe fd for HUP/ERR via
managedInstanceAlive and detachFull's dead instances. Adds liveness_timer_fd
to the supervisor poll set (SUPERVISOR_MAX_FDS 12 -> 13).
The Defect-2 re-attach call broke the -Dlibusb=false / musl-static build
because the compat stub header lacked the symbol. Mirror the existing
detach_kernel_driver stub.
…cts unplug

On LIBUSB_ERROR_NO_DEVICE the read loop only wrote a wake byte and left
pipe_w open, so a vendor/libusb device's pollfd never reported POLLHUP on
physical unplug. managedInstanceAlive judged the instance alive forever and
sweepLivenessLibusb skipped it, leaving a zombie emulated controller.

Mirror MockDeviceIO.closeWriteEnd: atomically xchg pipe_w to -1 and close it
once, from both the disconnect path and close(), guarding the double-close.
Add a falsifiable unit test asserting signalDisconnect raises POLLHUP.

Refs #355
An all-suppress device config opens no read fd (devices.len==0) yet holds
the device via libusb (suppress_devs non-empty). The liveness sweep read
managedInstanceAlive's len==0 shortcut as 'dead' and tore down the healthy
instance ~1s after attach, releasing the suppress claims and letting hidraw
nodes reappear — a self-inflicted ~1s flap loop.

Skip instances with no pollable read fd in sweepLivenessLibusb: unplug
detection there is POLLHUP-based and out of scope for fd-less instances.
Also reject zero-readable-interface configs in device validate() so an
all-suppress device is a hard config error rather than a latent footgun.

Refs #355
@BANANASJIM BANANASJIM marked this pull request as ready for review June 1, 2026 11:03
@qodo-code-review

Copy link
Copy Markdown

Review Summary by Qodo

Claim all Vader 5 HID interfaces via libusb to eliminate kernel hidraw nodes

✨ Enhancement 🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Claim all three Vader 5 HID interfaces via libusb to suppress kernel hidraw nodes
  - IF1 switched to vendor class for libusb transport (read/write)
  - IF2/IF3 claimed via new suppress class (evict kernel driver only)
• Add UsbrawSuppress for claim-only interface eviction without read threads
• Implement order-independent interface-to-device routing via helper functions
• Add liveness sweep timer to detect physical unplug of libusb-backed devices
• Extend validation to reject suppress interfaces with endpoints or report references
Diagram
flowchart LR
  A["Vader 5 Device<br/>3 HID Interfaces"] -->|IF1 vendor| B["UsbrawDevice<br/>Read/Write"]
  A -->|IF2 suppress| C["UsbrawSuppress<br/>Claim Only"]
  A -->|IF3 suppress| D["UsbrawSuppress<br/>Claim Only"]
  B --> E["devices[] slot"]
  C --> F["suppress_devs[] slice"]
  D --> F
  E --> G["No kernel hidraw node<br/>Steam cannot read"]
  F --> G

Loading

Grey Divider

File Changes

1. src/config/device.zig ✨ Enhancement +235/-2

Add suppress interface class with order-independent routing

• Add isSuppressClass() predicate and interface mapping helpers (openedInterfaceCount,
 deviceIndexForInterface, interfaceForDeviceIndex)
• Extend validate() to accept "suppress" class, reject suppress interfaces with endpoints, and
 reject reports/commands/init referencing suppress interfaces
• Require at least one non-suppress interface to prevent all-suppress configs
• Add comprehensive tests for suppress interface ordering, validation, and Vader 5 configuration

src/config/device.zig


2. src/device_instance.zig ✨ Enhancement +146/-7

Two-pass device open with separate suppress interface tracking

• Add suppress_devs field to store claim-only interfaces separately from readable devices
• Implement two-pass device open: Pass 1 opens hid/vendor interfaces into devices[], Pass 2 claims
 suppress interfaces into suppress_devs[]
• Update init sequence routing to use deviceIndexForInterface() helper for correct mapping
 regardless of TOML ordering
• Add cleanup for suppress devices in deinit()
• Add regression tests for suppress-first interface ordering and init routing

src/device_instance.zig


3. src/event_loop.zig ✨ Enhancement +7/-9

Use helper functions for order-independent interface routing

• Update resolveIfaceIdx() to use deviceIndexForInterface() helper for order-independent routing
• Update read loop interface lookup to use interfaceForDeviceIndex() helper instead of positional
 indexing

src/event_loop.zig


View more (4)
4. src/io/usbraw.zig ✨ Enhancement +93/-3

Add UsbrawSuppress for claim-only interface eviction

• Extract closeWriteEnd() and signalDisconnect() helper methods in UsbrawDevice
• Add libusb_attach_kernel_driver() call in close path to restore kernel driver
• Implement new UsbrawSuppress struct for claim-only interface eviction
• UsbrawSuppress.openSuppress() performs libusb init, device open, driver detach, and interface
 claim without spawning read thread
• UsbrawSuppress.close() releases interface and restores kernel driver
• Add test for signalDisconnect() pipe closure and POLLHUP signaling

src/io/usbraw.zig


5. src/supervisor.zig ✨ Enhancement +318/-6

Add liveness sweep for libusb-backed device unplug detection

• Add liveness_timer_fd recurring 1s timer to detect physical unplug of libusb-backed devices
• Implement instanceHoldsLibusb() to identify vendor-class or suppress-holding instances
• Update detach() to skip suspension for libusb instances (hidraw REMOVE is side effect of
 padctl's own claim)
• Implement drainLivenessTimer() and sweepLivenessLibusb() to tear down instances with hung-up
 device fds
• Update hotplug rebind to skip suppress interfaces when allocating new_devices array
• Add poll slot for liveness timer in SupervisorPollSet
• Add tests for libusb instance detection, detach behavior, and liveness sweep

src/supervisor.zig


6. compat/libusb-1.0/libusb.h Miscellaneous +3/-0

Add libusb_attach_kernel_driver stub

• Add stub libusb_attach_kernel_driver() function to mock libusb header

compat/libusb-1.0/libusb.h


7. devices/flydigi/vader5.toml ⚙️ Configuration changes +19/-4

Configure Vader 5 with vendor IF1 and suppress IF2/IF3

• Switch IF1 from "hid" to "vendor" class with explicit ep_in=0x82 and ep_out=0x06
• Add IF2 (id=2) and IF3 (id=3) as suppress-class interfaces for kernel driver eviction
• Extend block_kernel_drivers to include "hid_generic" and "usbhid" for proactive unbind
• Update comments to explain three-interface claim strategy and suppress-only purpose

devices/flydigi/vader5.toml


Grey Divider

Qodo Logo

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

Grey Divider

Looking for bugs?

Check back in a few minutes. An AI review agent is analyzing this pull request.

Grey Divider

Qodo Logo

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
src/config/device.zig (1)

309-320: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reject unknown interface IDs here as well.

This pre-pass only rejects suppress-class references. If a report.interface, commands.*.interface, or device.init.interface points at an ID that does not exist, isSuppressInterface() returns false and validation succeeds. The runtime helpers then resolve that ID to null, so the config fails silently instead of fail-closing.

Suggested fix
+fn findInterface(cfg: *const DeviceConfig, iface_id: i64) ?*const InterfaceConfig {
+    for (cfg.device.interface) |*iface| {
+        if (iface.id == iface_id) return iface;
+    }
+    return null;
+}
+
+fn validateUsableInterfaceRef(cfg: *const DeviceConfig, iface_id: i64) !void {
+    const iface = findInterface(cfg, iface_id) orelse return error.InvalidConfig;
+    if (isSuppressClass(iface.class)) return error.InvalidConfig;
+}
+
     for (cfg.report) |report| {
-        if (isSuppressInterface(cfg, report.interface)) return error.InvalidConfig;
+        try validateUsableInterfaceRef(cfg, report.interface);
     }
     if (cfg.commands) |cmds| {
         var it = cmds.map.iterator();
         while (it.next()) |entry| {
-            if (isSuppressInterface(cfg, entry.value_ptr.interface)) return error.InvalidConfig;
+            try validateUsableInterfaceRef(cfg, entry.value_ptr.interface);
         }
     }
     if (cfg.device.init) |init_cfg| {
         if (init_cfg.interface) |iface_id| {
-            if (isSuppressInterface(cfg, iface_id)) return error.InvalidConfig;
+            try validateUsableInterfaceRef(cfg, iface_id);
         }
     }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/config/device.zig` around lines 309 - 320, The validation currently only
rejects suppress-class references via isSuppressInterface but does not reject
unknown IDs; update the three spots (cfg.report entries, cfg.commands iterator
entries, and cfg.device.init.interface) so that after calling
isSuppressInterface(...) you also verify the referenced interface ID actually
exists and return error.InvalidConfig if it does not; use the same
interface-resolution helper the runtime uses (the project's interface
lookup/resolution function) to check existence when validating report.interface,
entry.value_ptr.interface, and init_cfg.interface.
src/io/usbraw.zig (1)

100-111: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Re-attach kernel driver on all post-detach failures in UsbrawDevice.open() / UsbrawSuppress.openSuppress()

src/io/usbraw.zig detaches the kernel driver, but the Busy/ClaimFailed return paths (lines ~100-111) and the later errdefer cleanup paths (lines ~134-139 and ~267-285 for UsbrawSuppress.openSuppress) only close/release—leaving the interface driverless until replug if the detach succeeded.

Suggested fix
-        _ = c.libusb_detach_kernel_driver(handle, interface_id);
+        const detached = c.libusb_detach_kernel_driver(handle, interface_id) == 0;
 
         const rc = c.libusb_claim_interface(handle, interface_id);
         if (rc == c.LIBUSB_ERROR_BUSY) {
+            if (detached) _ = c.libusb_attach_kernel_driver(handle, interface_id);
             c.libusb_close(handle);
             c.libusb_exit(ctx);
             return error.Busy;
         }
         if (rc != 0) {
+            if (detached) _ = c.libusb_attach_kernel_driver(handle, interface_id);
             c.libusb_close(handle);
             c.libusb_exit(ctx);
             return error.ClaimFailed;
         }
...
         errdefer {
             _ = c.libusb_release_interface(handle, interface_id);
+            if (detached) _ = c.libusb_attach_kernel_driver(handle, interface_id);
             c.libusb_close(handle);
             c.libusb_exit(ctx.?);
         }

Apply the same “track detached + attach in every post-detach failure/cleanup” pattern in UsbrawSuppress.openSuppress(), including the allocation-failure cleanup after a successful claim.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/io/usbraw.zig` around lines 100 - 111, The code detaches the kernel
driver but fails to re-attach it on all error/cleanup paths in
UsbrawDevice.open() and UsbrawSuppress.openSuppress(); update both functions to
track a boolean (e.g., detached) when libusb_detach_kernel_driver(handle,
interface_id) succeeds and ensure libusb_attach_kernel_driver(handle,
interface_id) is called on every subsequent failure/return and inside any
errdefer/cleanup blocks (including the allocation-failure path after a
successful claim and the Busy/ClaimFailed return paths) so the interface is
re-attached whenever detach had previously succeeded.
src/supervisor.zig (1)

519-523: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Check libusb ownership before the suspend_grace_sec == 0 fast path.

This immediate detachFull() runs before the libusb-backed-instance guard below, so setting suspend_grace_sec = 0 makes padctl tear down a live vendor/suppress device on its own claim-driven hidraw REMOVE.

Suggested fix
 pub fn detach(self: *Supervisor, devname: []const u8) void {
+    const peek = self.devname_map.get(devname) orelse {
+        std.log.debug("detach: {s} not in devname_map", .{devname});
+        return;
+    };
+    for (self.managed.items) |*m| {
+        if (!std.mem.eql(u8, m.phys_key, peek)) continue;
+        if (instanceHoldsLibusb(m)) {
+            std.log.debug("detach: {s} holds libusb; ignoring hidraw REMOVE", .{devname});
+            return;
+        }
+        break;
+    }
+
     if (self.suspend_grace_sec == 0) {
         // Grace window disabled: fall through to full teardown so the
         // uinput fd is released alongside the hidraw handle.
         self.detachFull(devname);
         return;
     }
-
-    const peek = self.devname_map.get(devname) orelse {
-        std.log.debug("detach: {s} not in devname_map", .{devname});
-        return;
-    };
-    for (self.managed.items) |*m| {
-        if (!std.mem.eql(u8, m.phys_key, peek)) continue;
-        if (instanceHoldsLibusb(m)) {
-            std.log.debug("detach: {s} holds libusb; ignoring hidraw REMOVE", .{devname});
-            return;
-        }
-        break;
-    }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/supervisor.zig` around lines 519 - 523, The fast-path that calls
self.detachFull(devname) when self.suspend_grace_sec == 0 can prematurely
teardown devices owned by libusb; modify the branch in supervisor.zig to first
check the libusb ownership/guard (the same condition used by the
libusb-backed-instance guard later in this function) and only call
self.detachFull(devname) when the device is not libusb-owned; i.e., perform the
libusb ownership check before the suspend_grace_sec == 0 fast path and skip or
defer detachFull for libusb-backed instances so the existing guard handles their
teardown.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/supervisor.zig`:
- Around line 649-651: The current unplug handler uses ManagedInstance.devname
(which is null for statically-spawned run() entries) and calls
self.detachFull(devname), so libusb-backed instances with null devname are never
reaped; update the teardown path to not depend on devname: detect when m.devname
is null and instead call a teardown routine that uses the instance index or
physical key (e.g., the instance's phys key / index stored on m.instance) to
remove the instance, or extend/overload detachFull to accept an identifier type
(devname OR phys key/index) and perform teardown by phys key when devname is
null; ensure you reference ManagedInstance.devname, m.instance (phys key/index),
and self.detachFull (or new detachByPhysKey) to locate and change the code.

---

Outside diff comments:
In `@src/config/device.zig`:
- Around line 309-320: The validation currently only rejects suppress-class
references via isSuppressInterface but does not reject unknown IDs; update the
three spots (cfg.report entries, cfg.commands iterator entries, and
cfg.device.init.interface) so that after calling isSuppressInterface(...) you
also verify the referenced interface ID actually exists and return
error.InvalidConfig if it does not; use the same interface-resolution helper the
runtime uses (the project's interface lookup/resolution function) to check
existence when validating report.interface, entry.value_ptr.interface, and
init_cfg.interface.

In `@src/io/usbraw.zig`:
- Around line 100-111: The code detaches the kernel driver but fails to
re-attach it on all error/cleanup paths in UsbrawDevice.open() and
UsbrawSuppress.openSuppress(); update both functions to track a boolean (e.g.,
detached) when libusb_detach_kernel_driver(handle, interface_id) succeeds and
ensure libusb_attach_kernel_driver(handle, interface_id) is called on every
subsequent failure/return and inside any errdefer/cleanup blocks (including the
allocation-failure path after a successful claim and the Busy/ClaimFailed return
paths) so the interface is re-attached whenever detach had previously succeeded.

In `@src/supervisor.zig`:
- Around line 519-523: The fast-path that calls self.detachFull(devname) when
self.suspend_grace_sec == 0 can prematurely teardown devices owned by libusb;
modify the branch in supervisor.zig to first check the libusb ownership/guard
(the same condition used by the libusb-backed-instance guard later in this
function) and only call self.detachFull(devname) when the device is not
libusb-owned; i.e., perform the libusb ownership check before the
suspend_grace_sec == 0 fast path and skip or defer detachFull for libusb-backed
instances so the existing guard handles their teardown.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ed345c0e-1adf-4537-9889-82428ac24578

📥 Commits

Reviewing files that changed from the base of the PR and between 61c09e8 and ab09917.

📒 Files selected for processing (4)
  • compat/libusb-1.0/libusb.h
  • src/config/device.zig
  • src/io/usbraw.zig
  • src/supervisor.zig

Comment thread src/supervisor.zig Outdated
BANANASJIM added 2 commits June 2, 2026 00:53
- usbraw: on libusb_claim_interface failure (BUSY or other), re-attach the
  kernel driver before closing the handle so a detached-then-failed
  interface is not left driverless (UsbrawDevice.open + UsbrawSuppress).
- device: add an isolated test proving the report->suppress referential
  check rejects even when a readable interface is present (the prior test
  was shadowed by the all-suppress openedInterfaceCount==0 guard).
- supervisor: restore the managedInstanceAlive doc comment that was
  mis-attached to instanceHoldsLibusb on insertion.
- device_instance: drop the stray investigation marker from a test comment.

Refs #355
…ess sweep

The sweep gated teardown on `m.devname orelse continue`, so a libusb-backed
instance spawned without a devname (the spawnInstance path used by the
test-only run() and doReload's found==null branch) was never torn down on
unplug. Tear it down by phys key when devname is null, mirroring detachFull
(stop + join + quiesce + teardownManaged + swapRemove). Adds a falsifiable
regression test.

Refs #355
@BANANASJIM BANANASJIM merged commit 731bc17 into main Jun 2, 2026
36 checks passed
@BANANASJIM BANANASJIM deleted the fix/vader5-libusb-claim-355 branch June 2, 2026 09:21
BANANASJIM added a commit that referenced this pull request Jun 3, 2026
)

PR #357 moved the Vader 5 to libusb (vendor/suppress interfaces) but the installer granted only hidraw/input node access, not the raw USB device node — so the unprivileged user daemon could not claim the device and the bind failed silently. Emit a SUBSYSTEM=="usb" uaccess/GROUP/MODE grant for libusb-claimed devices, and log an actionable message on claim failure instead of dropping the device silently.

Refs #355
BANANASJIM added a commit that referenced this pull request Jun 4, 2026
padctl self-detaches the kernel HID driver when it libusb-claims IF1/IF2/IF3, so blocking hid_generic/usbhid at udev was redundant and removed the hidraw node that hidraw-only discovery depends on (#355 regression). Block only xpad: usbhid stays bound so a hidraw node exists for discovery, and the libusb claim still hides the pad from Steam. Verified on real Flydigi Vader 5 hardware. Refs #355, #357.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant