fix(install): grant raw USB node access for libusb-claimed devices (#355)#361
Merged
Conversation
) PR #357 moved the Vader 5 from hidraw (class="hid") to libusb (class="vendor"/"suppress") so the kernel exposes no hidraw node for Steam to grab. libusb then needs write access to the raw USB device node (/dev/bus/usb/...) plus driver detach, but the installer only granted the hidraw/input nodes — so the unprivileged --user daemon could not claim the device and the bind failed, leaving `padctl status` empty. - udev: emit a SUBSYSTEM=="usb", ENV{DEVTYPE}=="usb_device" uaccess/ GROUP/MODE grant for any device that declares a vendor/suppress interface (detected from the device TOML at install time) - supervisor: a libusb claim failure no longer drops the device with a generic warning; log an actionable message naming the device and remedy - tests: udev rule emitted for vendor/suppress devices and absent for hidraw-only devices; usesLibusb helper coverage Refs #355
📝 WalkthroughWalkthroughThis PR implements end-to-end support for generating udev rules that grant raw USB access to devices requiring libusb. It detects vendor/suppress interface classes in device configs, tracks this requirement through udev rule generation, emits appropriate usb-subsystem rules with uaccess tags, and improves error messages when libusb device binding fails. ChangesLibusb udev rules and error messaging
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Review Summary by QodoGrant raw USB node access for libusb-claimed devices
WalkthroughsDescription• Grant raw USB device node access for libusb-claimed devices via udev rules - Emit SUBSYSTEM=="usb" rules for vendor/suppress interface devices - Detect libusb usage from device TOML at install time • Improve error messaging for libusb device binding failures - Log actionable diagnostics naming the device and remedy - Distinguish libusb claim errors from generic initialization failures • Add comprehensive test coverage for udev rule generation - Verify USB node rules emitted for libusb devices - Verify USB node rules absent for hidraw-only devices - Add usesLibusb helper function with test coverage Diagramflowchart LR
A["Device TOML<br/>vendor/suppress class"] -->|detectLibusb| B["UdevEntry<br/>needs_libusb flag"]
B -->|generateRules| C["USB node rule<br/>SUBSYSTEM==usb"]
D["DeviceInstance.init<br/>libusb claim fails"] -->|isLibusbClaimError| E["logBindFailure<br/>actionable message"]
E -->|user sees| F["Install udev rules<br/>join input group"]
File Changes1. src/cli/install/tests.zig
|
Code Review by Qodo
Context used 1. Libusb Busy not retried
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/cli/install/tests.zig (1)
369-369: 💤 Low valueTest constant missing
ACTION=="add",prefix.The generated rule (line 452 of udev.zig) includes
ACTION=="add",at the start, but this test constant omits it. The test still passes via substring matching, but won't catch if the ACTION clause is accidentally removed from generation.🔧 Suggested fix
-const usb_node_rule = "SUBSYSTEM==\"usb\", ENV{DEVTYPE}==\"usb_device\", ATTR{idVendor}==\"37d7\", ATTR{idProduct}==\"2401\", TAG+=\"uaccess\", GROUP=\"input\", MODE=\"0660\""; +const usb_node_rule = "ACTION==\"add\", SUBSYSTEM==\"usb\", ENV{DEVTYPE}==\"usb_device\", ATTR{idVendor}==\"37d7\", ATTR{idProduct}==\"2401\", TAG+=\"uaccess\", GROUP=\"input\", MODE=\"0660\"";🤖 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/cli/install/tests.zig` at line 369, The test constant usb_node_rule is missing the leading ACTION=="add", clause that the generated udev rule in udev.zig expects; update the usb_node_rule string to include ACTION=="add", at the start (so it exactly matches the generated rule including the ACTION clause) to ensure the test fails if the ACTION clause is omitted by the generator.
🤖 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/cli/install/tests.zig`:
- Line 369: The test constant usb_node_rule is missing the leading
ACTION=="add", clause that the generated udev rule in udev.zig expects; update
the usb_node_rule string to include ACTION=="add", at the start (so it exactly
matches the generated rule including the ACTION clause) to ensure the test fails
if the ACTION clause is omitted by the generator.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eb78c9c0-a48e-4419-8975-4a735ca9e48f
📒 Files selected for processing (4)
src/cli/install/tests.zigsrc/cli/install/udev.zigsrc/config/device.zigsrc/supervisor.zig
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+2317
to
+2321
| fn isLibusbClaimError(err: anyerror) bool { | ||
| return switch (err) { | ||
| error.NotFound, error.Busy, error.ClaimFailed => true, | ||
| else => false, | ||
| }; |
There was a problem hiding this comment.
1. Libusb busy not retried 🐞 Bug ☼ Reliability
isLibusbClaimError now treats error.Busy as a libusb claim failure, but the retry path still depends on isTransientOpenError; since isTransientOpenError does not include error.Busy, a LIBUSB_ERROR_BUSY bind failure will log and then drop without scheduling a retry. This can leave a device missing from padctl status until the user manually replugs/restarts the daemon.
Agent Prompt
## Issue description
`usbraw` can return `error.Busy` (mapped from `LIBUSB_ERROR_BUSY`). The supervisor currently enqueues a hotplug retry only when `isTransientOpenError(err)` is true; `error.Busy` is not considered transient there, so a busy libusb claim can cause a permanent drop until user intervention.
## Issue Context
This PR adds `isLibusbClaimError()` (including `error.Busy`) and `logBindFailure()` for libusb devices, but does not adjust the retry classification to match.
## Fix Focus Areas
- src/supervisor.zig[2300-2315]
- src/supervisor.zig[2317-2338]
- src/io/usbraw.zig[95-114]
### Suggested fix
Add `error.Busy` to `Supervisor.isTransientOpenError()` (or map `usbraw`'s busy condition to an existing transient error like `error.DeviceBusy`) so the existing retry path (`enqueueHotplugRetryForPath`) also covers libusb busy claim failures.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
PR #357 moved the Flydigi Vader 5 from hidraw (
class="hid") to libusb (class="vendor"/"suppress") so the kernel exposes no hidraw node for Steam to grab. libusb then needs write access to the raw USB device node (/dev/bus/usb/...) pluslibusb_detach_kernel_driver— but the installer only granted the hidraw/input nodes. So an unprivilegedsystemd --userdaemon could not claim the device: the bind failed andpadctl statusshowed nothing (reported on #355 after building 731bc17).Empirically confirmed: the generated udev rules granted
hidraw+inputfor37d7:2401but noSUBSYSTEM=="usb"permission; raw USB nodes areroot:root 0664, so a non-root daemon cannot open them.Changes
udev.zig): emitACTION=="add", SUBSYSTEM=="usb", ENV{DEVTYPE}=="usb_device", ATTR{idVendor}==.., ATTR{idProduct}==.., TAG+="uaccess", GROUP="input", MODE="0660"for any device that declares avendor/suppressinterface (detected from the device TOML at install time). Mirrors the existing hidraw grant; emitted only for libusb devices (exactly 1 today: the Vader).supervisor.zig): a libusb claim failure no longer drops the device with a generic warning —logBindFailurelogs an actionable message naming the device and the remedy (install udev rules / joininputgroup / run privileged).usesLibusbhelper coverage.Verification
37d7:2401(exactly one, no HID device affected).zig build testgreen in Docker;zig fmt --checkclean (0.15.2); musl-Dlibusb=falsebuild green.ATTRnotATTRS, gated onDEVTYPE=="usb_device"; both catch sites keep their transient-retry; nostd.log.errin test-exercised paths; new tests are falsifiable).Known limitation (not addressed here)
#357 also changed
block_kernel_driversfrom["xpad"]to["xpad","hid_generic","usbhid"], so udev now eagerly unbinds all HID drivers on plug, removing the hidraw node. Discovery is still hidraw-only (no libusb enumeration fallback), so there is a likely race where the node is gone before the daemon reads it. This is unconfirmed without hardware and not changed here; the new actionable log will help disambiguate it from the permission gap in the field.Refs #355
Summary by CodeRabbit
New Features
Tests
Improvements