Fix host USB HID boot, mode, and recovery bugs - #3
Closed
mjc wants to merge 4 commits into
Closed
Conversation
|
Lost in the diff? Review this PR in Change Stack to follow the change map from intent to exact ranges. 📝 WalkthroughWalkthroughRefactors USB gadget init into a shared helper script, updates S03usbdev and S03usbhid to use it, adds an integration test suite that simulates configfs/sysfs, and adjusts Go/C++ HID handling and tests. ChangesUSB Gadget Initialization Refactoring and Testing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
This was referenced Jun 5, 2026
mjc
force-pushed
the
mjc/fix-usb-hid-boot-subclass
branch
from
June 5, 2026 19:38
88594bc to
eeb55d5
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eeb55d5dc1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74a1f917c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
mjc
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@kvmapp/system/init.d/S03usb-common`:
- Around line 82-95: The UDC binding code in bind_first_udc and restart_udc
writes every basename from ${USB_UDC_CLASS} into UDC which can produce a
multi-line value; change both locations to select exactly one UDC name (e.g.,
pipe the find/basename output through head -n1) and write only that single name
to the UDC target (use "${USB_GADGET_ROOT}/${USB_GADGET_NAME}/UDC" in
restart_udc and UDC file in bind_first_udc); additionally add a simple check
that the selected name is non-empty (and optionally log/exit when none found or
when more than one entry exists) so binding never receives a multi-line value.
In `@tests/usb-init-scripts-test.sh`:
- Around line 201-237: Add a regression test that exercises the production
stop→start lifecycle: create a new test function (e.g.,
test_stop_then_start_rebinds_udc) that uses new_env and run_start to initialize
the gadget tree, then invoke sh "${NORMAL_SCRIPT}" stop and then sh
"${NORMAL_SCRIPT}" start (with USB_TEST_MODE, USB_COMMON, USB_BOOT_DIR,
USB_GADGET_ROOT, USB_UDC_CLASS, and USB_OTG_ROLE set as in other tests) and
assert the gadget was reconfigured (check "${base}/gadget/g0/UDC" equals the
expected UDC string like "4340000.usb" and "${base}/proc/cviusb/otg_role" is set
appropriately) so the suite covers start after an already-initialized gadget.
- Around line 173-199: The test test_hid_only_descriptors_and_no_wake only
checks the serial string; add assertions to verify the full English string set
(manufacturer, product, and configuration) are written in HID-only mode by
asserting "${g}/strings/0x409/manufacturer" and "${g}/strings/0x409/product" and
"${g}/configs/c.1/strings/0x409/configuration" equal the same expected values
used by the normal-mode test (i.e., copy the assert_eq lines for
manufacturer/product/configuration from the normal-mode test into
test_hid_only_descriptors_and_no_wake so HID-only uses identical strings).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fa6b6393-7d6c-49e0-8319-6adc2e1da9f8
📒 Files selected for processing (8)
Makefilekvmapp/system/init.d/S03usb-commonkvmapp/system/init.d/S03usbdevkvmapp/system/init.d/S03usbhidserver/service/hid/status.goserver/service/hid/status_test.gosupport/sg2002/kvm_system/main/lib/system_state/system_state.cpptests/usb-init-scripts-test.sh
mjc
force-pushed
the
mjc/fix-usb-hid-boot-subclass
branch
from
June 5, 2026 21:35
74bebf4 to
4dcaa87
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@kvmapp/system/init.d/S03usb-common`:
- Around line 272-278: The restart_usb_phy function calls start_usb_dev which
may not be defined by every script sourcing S03usb-common; add a guard to check
for the existence of start_usb_dev before calling it (e.g., use a conditional
like command -v or type to test start_usb_dev) and log or return an error if
it's missing, or alternatively document in the file header that any script
sourcing S03usb-common must define start_usb_dev; update restart_usb_phy to call
start_usb_dev only when present to avoid silent failures.
In `@kvmapp/system/init.d/S03usbdev`:
- Around line 43-54: The code redundantly reads "${USB_BOOT_DIR}/usb.disk0"
twice; reuse the previously read variable disk instead of catting the file again
— in the S03usbdev script, inside the if/else that references
USB_MASS_STORAGE_FUNC and lun.0/file, replace the second cat
"${USB_BOOT_DIR}/usb.disk0" usage with the $disk variable so the else branch
writes the already-read content to
"functions/${USB_MASS_STORAGE_FUNC}/lun.0/file".
In `@tests/usb-init-scripts-test.sh`:
- Around line 270-281: The test test_restart_phy_rebuilds_gadget relies on an
implicit equality between USB_PHY_DEVICE and USB_DEFAULT_UDC; make this explicit
by setting USB_PHY_DEVICE to the expected USB_DEFAULT_UDC value when invoking
run_script_action (or define a dedicated test constant) so restart_phy writes
the same device string the assertions expect; update the run_script_action call
or the test setup to export or pass USB_PHY_DEVICE="4340000.usb" (or use a named
constant) before calling restart_phy so the checks against dwc2/unbind,
dwc2/bind and g/UDC are deterministic.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6aa367b7-148e-40c7-9d2e-4222951e65fe
📒 Files selected for processing (4)
kvmapp/system/init.d/S03usb-commonkvmapp/system/init.d/S03usbdevkvmapp/system/init.d/S03usbhidtests/usb-init-scripts-test.sh
mjc
force-pushed
the
mjc/fix-usb-hid-boot-subclass
branch
from
June 5, 2026 21:39
4dcaa87 to
2e1bfa9
Compare
mjc
force-pushed
the
mjc/fix-usb-hid-boot-subclass
branch
from
June 5, 2026 21:42
2e1bfa9 to
964eb9b
Compare
mjc
force-pushed
the
mjc/fix-usb-hid-boot-subclass
branch
from
June 5, 2026 22:03
f903df6 to
5b0ac1c
Compare
Owner
Author
|
Closing this fork-local PR; reopening the same branch against upstream sipeed/NanoKVM. |
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
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.
Summary
This PR fixes host-visible USB HID failures where NanoKVM keyboard and mouse input can fail in BIOS/UEFI, boot menus, GRUB, or after USB gadget recovery.
The core fixes are:
/dev/hidg*handles after the gadget is rebuiltThis PR is intentionally limited to HID and USB descriptor/recovery bugs. It does not change virtual media behavior or UI.
Problem
The existing normal and HID-only init scripts build USB gadgets through separate, duplicated paths. That allowed HID descriptor behavior to drift between modes and made firmware/host enumeration sensitive to boot flags and previous host state.
The main failure modes addressed here:
/dev/hidg*handles on writeWhat Changed
S03usb-commonfor shared USB gadget setup.S03usbdevandS03usbhidto use the shared helper./boot/usb.notwakeup.startrebuild an existing gadget tree afterstop.shwith a timeout.hid.GS0,hid.GS1, andhid.GS2config links./dev/hidg*handles after gadget rebuilds.Related Issues
Validation
Automated checks:
Additional shell compatibility checks were run against the USB init script harness with:
The fake configfs harness verifies:
/boot/BIOSno longer changes the intended HID layout/boot/disable_hidremoves HID functions in normal mode/boot/usb.notwakeuprestart_phyflows work through the shared helperLive validation confirmed that cold firmware setup accepts NanoKVM keyboard input with this branch, and that normal Linux enumeration binds all three HID interfaces without
usbhid/hid-genericerrors.