Fix host-visible USB virtual media behavior#4
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR refactors host-visible USB gadget handling to eliminate the “fake media” /dev/mmcblk0p3 fallback, separating selected virtual media (disk0) from the optional host-visible data disk export (disk1), and centralizing runtime gadget mutations behind a new Go USB helper.
Changes:
- Introduces
server/service/usbgadget helper to detach/attach UDC around safe LUN/function mutations and to toggle RNDIS / virtual media / data disk independently. - Updates boot-time init scripts and server APIs to use
usb.media0for selected virtual media enablement, while exporting the NanoKVM data partition only viamass_storage.disk1when enabled. - Adds/updates regression and unit tests, plus a
make testtarget to run both script and Go server test suites.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/usb-init-scripts-test.sh | Updates regression expectations for media/data-disk split and new boot flags. |
| support/sg2002/kvm_system/main/lib/system_state/system_state.cpp | Improves SG2002 USB status detection by checking concrete configfs links and adds safer UDC state open handling. |
| server/service/vm/virtual-device.go | Switches virtual device toggles from shell scripts to the new USB gadget helper; adds “media” toggle. |
| server/service/usb/gadget.go | New centralized USB gadget helper for UDC detach/attach, function/link toggles, and LUN image state. |
| server/service/usb/gadget_test.go | Adds fake-configfs unit tests covering helper behavior, independence, and error paths. |
| server/service/storage/image.go | Routes mount/unmount and status queries through the USB gadget helper (no-media now clears LUN file). |
| server/service/hid/status.go | Reuses USB helper paths and PHY restart logic; removes duplicated reset sequence. |
| server/service/hid/status_test.go | Adds tests for HID mode detection using overridable usb.ModeFlag. |
| Makefile | Adds make test and make test-server targets; preserves make test-usb-scripts. |
| kvmapp/system/init.d/S03usbdev | Updates boot-time gadget composition for usb.media0 (disk0) and usb.disk0 data disk (disk1). |
| kvmapp/system/init.d/S03usb-common | Refactors common helpers (string/config string/wakeup) without behavior change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| FILE *fp; | ||
| int file_size; | ||
| uint8_t RW_Data[10]; | ||
| fp = fopen("/sys/class/udc/4340000.usb/state", "r"); | ||
| fp = fopen(USB_UDC_STATE, "r"); | ||
| if(fp == NULL) { |
![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: 877aa7d6aa
ℹ️ 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".
| if [ -e "${USB_BOOT_DIR}/usb.disk0" ] | ||
| then | ||
| mkdir functions/mass_storage.disk1 | ||
| prepare_fake_lun mass_storage.disk1 | ||
| ln -s functions/mass_storage.disk1 configs/c.1/ | ||
| echo 1 > functions/mass_storage.disk1/lun.0/removable | ||
| printf '%s' "NanoKVM USB Data Disk0520" > functions/mass_storage.disk1/lun.0/inquiry_string | ||
| echo /dev/mmcblk0p3 > functions/mass_storage.disk1/lun.0/file |
There was a problem hiding this comment.
On upgraded devices that already have /boot/usb.disk0 populated with an image path from the previous boot-time media behavior, this new branch treats that legacy media flag as the data-disk toggle and unconditionally exposes /dev/mmcblk0p3, while the media branch above only reads the new /boot/usb.media0 name. In that upgrade scenario the selected image is dropped and the host sees the NanoKVM data partition instead; migrate or distinguish non-empty legacy usb.disk0 before using it as the data-disk flag.
Useful? React with 👍 / 👎.
| func SetLUNImage(h HIDController, image string, cdrom bool) error { | ||
| image = NormalizeImage(image) | ||
| return WithDetachedUDC(h, func() error { | ||
| if err := EnsureFile(MassStorageFlag); err != nil { |
There was a problem hiding this comment.
When a fresh device uses only the virtual-media flow, SetLUNImage persists /boot/usb.media0, but the boot filesystem script still creates /dev/mmcblk0p3 only when /boot/usb.disk0 exists (kvmapp/system/init.d/S01fs:21). In that scenario the /data partition used by the image list/download paths is never created or mounted unless the separate data-disk export is also enabled, so media-only users can lose the storage area needed for ISOs/images after reboot.
Useful? React with 👍 / 👎.
| if err := EnsureFile(MassStorageFlag); err != nil { | ||
| return err | ||
| } | ||
| if err := ensureMassStorageLink(); err != nil { | ||
| return err |
There was a problem hiding this comment.
When the active gadget is in HID-only mode, S03usbhid composes only HID functions, but image mounting now calls SetLUNImage and these lines unconditionally create/link mass_storage.disk0. Since the image menu is still reachable in HID-only mode, a user can expose virtual storage despite selecting HID-only; previously this path would fail without an existing LUN. Check the current HID mode before ensuring media functions.
Useful? React with 👍 / 👎.
| errs := []error{ | ||
| RemoveIfExists(MassStorageLink), | ||
| RemoveIfExists(MassStorageFlag), | ||
| } | ||
| if Exists(LUNFile) { | ||
| errs = append([]error{DetachLUN()}, errs...) | ||
| } | ||
| return errors.Join(errs...) |
| @@ -86,7 +86,6 @@ test_normal_hid_descriptors(){ | |||
| assert_hex "${g}/strings/0x409/manufacturer" "736970656564" | |||
| assert_hex "${g}/strings/0x409/product" "4e616e6f4b564d" | |||
| assert_hex "${g}/strings/0x409/serialnumber" "30313233343536373839414243444546" | |||
|
|
||
| assert_eq "$(cat "${g}/bcdUSB")" "0x0110" "hid-only bcdUSB" | ||
| assert_eq "$(cat "${g}/bcdDevice")" "0x0623" "hid-only bcdDevice" | ||
| assert_eq "$(cat "${g}/configs/c.1/bmAttributes")" "0xA0" "hid-only bmAttributes" |
88594bc to
eeb55d5
Compare
877aa7d to
288422b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 288422bc36
ℹ️ 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".
| if err := ensureRNDISFunction(); err != nil { | ||
| return err | ||
| } | ||
| if !Exists(RNDISLink) { | ||
| return os.Symlink(RNDISFunction, RNDISLink) |
There was a problem hiding this comment.
On devices configured with /boot/usb.ncm, the init script explicitly creates ncm.usb0 instead of rndis.usb0 (S03usbdev checks usb.ncm before usb.rndis0). This runtime path ignores that flag and always creates/links the RNDIS function, so toggling the Network switch exposes a different USB network gadget until the next reboot/restart. Check the NCM flag and create the same function the boot script would use.
Useful? React with 👍 / 👎.
f903df6 to
5b0ac1c
Compare
288422b to
f16e3c2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f16e3c275b
ℹ️ 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".
| if err := EnsureFile(MassStorageFlag); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
When a user mounts an image through the API, this only creates /boot/usb.media0 and updates the live LUN; it never writes the normalized image into the boot flag. The updated init script restores virtual media after reboot by reading /boot/usb.media0, so API-mounted images either disappear on reboot when the file is empty or an older manually written path gets remounted instead. Write the selected image path to MassStorageFlag (and clear it on unmount) before reattaching the gadget.
Useful? React with 👍 / 👎.
| detachErr := DetachUDC() | ||
| mutateErr := mutate() | ||
| attachErr := AttachUDC() | ||
| openErr := h.OpenNoLockWithRetry(hidReopenTimeout, hidReopenDelay) |
There was a problem hiding this comment.
On devices booted in normal mode with /boot/disable_hid, S03usbdev intentionally skips creating the HID functions while the virtual device switches remain reachable because bcdDevice still reports normal mode. After toggling network/media/disk, reopening /dev/hidg* now becomes part of the returned error, so the API reports operation failed even though the gadget mutation already happened; missing HID devices in this configured mode should not make non-HID gadget updates fail.
Useful? React with 👍 / 👎.
| return err | ||
| } | ||
|
|
||
| if err := DetachLUN(); err != nil { |
There was a problem hiding this comment.
When a mount request names a backing file that no longer exists (for example a stale image list or a concurrent delete), DetachLUN ejects the currently mounted image before the later write to LUNFile is the first operation that can fail for that missing path. The handler then reports mount image failed, but the host has already lost the previous media while the UI still believes it is mounted; validate the requested backing file before detaching, or otherwise avoid clearing the current LUN until the replacement can succeed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0c0efeb84
ℹ️ 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".
| DataDiskFunction = GadgetPath + "/functions/mass_storage.disk0" | ||
| DataDiskLink = ConfigPath + "/mass_storage.disk0" |
There was a problem hiding this comment.
Because the data-disk paths are aliases for mass_storage.disk0, the data-disk toggle manipulates the same configfs function and LUN as virtual media instead of the separate mass_storage.disk1 export described by this change. When a user enables the data disk while an ISO/image is mounted, or mounts an image while the data disk is exposed, one backing file necessarily replaces the other and the opposite boot flag is removed, so the two host-visible devices cannot coexist.
Useful? React with 👍 / 👎.
a65a15b to
3d3c5e0
Compare
3d3c5e0 to
04e0aa5
Compare
f36ee45 to
295cffb
Compare
Context
This PR fixes host-visible USB virtual media behavior, stacked on top of the HID/descriptor fix branch.
The key bug is that NanoKVM used one overloaded
usb.disk0state for both selected virtual media and the NanoKVM data partition fallback. When no image was selected, the gadget could expose/dev/mmcblk0p3as fake media. That makes the host see media that is not actually mounted by the user, and it can confuse firmware, OS installers, and eject/remount flows.What changed
server/service/usbas the backend helper for host-visible USB gadget operations:forced_ejectusemass_storage.disk0is the selected virtual image devicelun.0/fileis empty, not/dev/mmcblk0p3/dev/mmcblk0p3is treated as legacy "no image" when reporting mounted image statero,cdrom, inquiry string, and backing file changes happen while the UDC is detachedforced_ejectis used only when the kernel exposes itmass_storage.disk1is the data-disk export/dev/mmcblk0p3only when that separate disk toggle is enabledaccess().forced_eject, legacy/dev/mmcblk0p3state, HID mode reporting, and boot-script media composition.make testfor the host USB script and server tests.Scope table
mass_storage.disk0/lun.0/fileempty./dev/mmcblk0p3fallback/dev/mmcblk0p3as the explicit data-disk export when that separate function is enabled.ro, andcdromchanges.forced_ejectwhen the kernel exposes it, otherwise clears the LUN backing file.forced_ejectcontinue to use the clear-file path.make test.Related upstream PRs fixed here
/dev/mmcblk0p3as no mounted image, and adds tests.Related reports fixed here
/dev/mmcblk0p3; this PR stops using that partition as fake no-image media.forced_ejectwhen available and otherwise clearslun.0/file.Validation
Validated locally with the Nix dev shell from the companion
mjc/setup-nix-build-envbranch:nix develop /home/mjc/projects/NanoKVM-nix-build-env -c make -C /home/mjc/projects/NanoKVM testAlso validated from the stacked cleanup branch with: