Skip to content

Virtual Media Changes #489

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

tpretz
Copy link
Contributor

@tpretz tpretz commented May 4, 2025

This PR's goal is to help address issues i have, only with specific hardware, losing HID connectivity when virtual media is inserted.

As part of debugging this problem, i split the mounting of SD card (virtual media toggle in settings) from iso virtual media.

This allows for SD card mounting to remain off (most cases i suspect people won't require it).
But still inject CDROM / boot media as required.

This change also removes the full reset of the USB gadget when boot media is inserted / ejected

I have tested, but only on the one device where i have had issues, and it seems to resolve them in most cases.
Multiple mass storage devices when both SD and boot media are attached.
Boot media can be changed without "replug" events on hid devices.

Unmount of media thats in use does still fail (as it did before) and may require a hid reset to force.

I can see a fix has been merged https://patchwork.kernel.org/project/linux-usb/patch/[email protected]/ which would allow for reliable unmounting of media. However thats not available in the current kernel version used.

@scpcom
Copy link
Contributor

scpcom commented May 17, 2025

What is the goal for the flag "/boot/usb.disk0.disabled"?
Why not just remove /boot/usb.disk0 to disable it? I think this part should not be changed.
https://github.com/sipeed/NanoKVM/pull/489/files#diff-d83f162ab71569674404ee340cd8f5e4ee33f2d3c0465dd04e64a14dd8b1568dL85

@tpretz
Copy link
Contributor Author

tpretz commented May 23, 2025

What is the goal for the flag "/boot/usb.disk0.disabled"? Why not just remove /boot/usb.disk0 to disable it? I think this part should not be changed. https://github.com/sipeed/NanoKVM/pull/489/files#diff-d83f162ab71569674404ee340cd8f5e4ee33f2d3c0465dd04e64a14dd8b1568dL85

Was trying to think, why did i do that ...
But, have remembered.

I don't know all of how /boot/usb.disk0 is created, i could see it was created in virtual-device.go

But, now it won't be, as that flag / logic will manage usb.disk1 instead.
I did not want a case where that was the only place creating the file and this may work for upgrades (if the file already exists), but not for fresh installations.

Additionally, i did not want to force enable the device, in the event an end-user wants to intentionally disable it.

So, i gave the end-user an option to intentionally disable it.
Then, if they haven't, i make sure the file at least exists.

Hopefully that all makes sense, it was in consideration of both fresh / upgrade paths given the changes in virtual-device.go

@scpcom
Copy link
Contributor

scpcom commented May 23, 2025

All flags in the /boot directory are user flags. The use should be able to control them via web interface or by manually creating/removing the file.
Adding "/boot/usb.disk0.disabled" does not solve the problem because we would get two unmanaged flags.
I think /boot/usb.disk0 and /boot/usb.disk1 should be managed both in virtual-device.go by adding a second function and a menu entry for mounting/unmonting the /data disk.

@tpretz
Copy link
Contributor Author

tpretz commented May 24, 2025

sure, sounds good, ill make the changes

@tpretz tpretz marked this pull request as draft May 24, 2025 11:49
@tpretz tpretz force-pushed the virtual-media-split branch from 2105cb5 to 86fc3b9 Compare May 24, 2025 11:49
backend config for media control

remove .disabled

adding settings toggle

add default text
@tpretz tpretz force-pushed the virtual-media-split branch from 6c98e87 to a1df637 Compare May 24, 2025 12:29
@tpretz tpretz marked this pull request as ready for review May 24, 2025 12:31
@scpcom
Copy link
Contributor

scpcom commented May 25, 2025

I compiled and tested it on my hardware.
Looks good, I found only two issues:

tpretz@a1df637#diff-50bcb645637346967e20608b5b6ebf3f3e6e1ddc540587458ece34def162dc29R19

The kernel does not accept empty image, this can be solved by using a new line (like echo does in the shell):
+ imageNone = "\n"

tpretz@a1df637#diff-50bcb645637346967e20608b5b6ebf3f3e6e1ddc540587458ece34def162dc29L108

Enabling CD-ROM mode does not work after reboot.
We need to keep usb reset for this (maybe only if image flags change).

@tpretz
Copy link
Contributor Author

tpretz commented May 25, 2025

That second problem would be a shame, ill take a look into it.

I would really like to avoid resetting the USB gadget, partly due to some bad bios behaviors on some of my hardware, but also because, it would reset the SD card media device that could be in-use and ideally would not be disrupted by changes in virtual media attachment.

@tpretz
Copy link
Contributor Author

tpretz commented May 25, 2025

Can you expand on the issues with cdrom mode after reboot?

Testing the USB gadget behavior manually, to see what its limitations are, i have swapped from cdrom to non-cdrom (and ro off and on) without needing to reset anything (apart from making sure "file" is unset before changing the cdrom and ro options).

Virtual media won't be re-mounted after KVM reboot.
I did think of enabling this (having the server application update the /boot files) however did not want to creep the scope of this PR too far.

I wonder if the imageNone problem is related, as if the image is not unmounted first then the change of cdrom and ro flags won't work. (ah, perhaps not there is an explicit newline written before)

set imageNone to newline
@scpcom
Copy link
Contributor

scpcom commented May 25, 2025

Yes, i am aware of "Virtual media won't be re-mounted after KVM reboot." and this can be handled in a different PR/issue, not here.

The cdrom issue can be easily reproduced:

  • Reboot NanoKVM
  • Login
  • Click the CD-ROM switch to enable it
  • Select a .iso image (in my case clonezilla)

Windows on target machine will recognize this but it is show as "unformatted" disk.
Flipping CD-ROM switch on/off or ejecting/inserting the ISO again does not help.
Flipping the Virtual Image switch in Device section does help as a workaround.
Re-enable the usb reset code does also help.

@tpretz
Copy link
Contributor Author

tpretz commented May 25, 2025

Can't seem to reproduce it myself
took a 2.2.8 base, same as what you are running ?

  • copied in the new S03usbdev to /etc/init.d
  • copied in the new NanoKVM-Server
  • (didn't bother with web, as its just settings toggle changes)
  • rebooted
  • attach and detach iso in cdrom mode
  • attach it in non-cdrom mode (and i get the normal "unformatted")

what do you have matching /boot/usb.* ?

although as long as /boot/usb.disk0 exists that should be sufficient to create whats needed for the server
... then all that is doing is, clearing file, setting cdrom / ro, setting file which all should be ok to set in that order

i will clean up a little more in image.go, not that it should have any impact on functionality

@scpcom
Copy link
Contributor

scpcom commented May 25, 2025

I build NanoKVM from sources (with my buildroot) and replace the full /kvmapp folder and the init.d scripts.
I will try your new patch.

@tpretz
Copy link
Contributor Author

tpretz commented May 25, 2025

i don't think that will make any difference, it was just cleanup of unused code

Ill do a fresh build from img on a test device tomorrow.
Perhaps there was something lingering on this one making it work.
Although, the actual changes to how the usb gadget is managed are fairly small, so it is strange.

No errors logged ?, especially around the "unmount image failed" ?
As sometimes a host OS will hold onto an image and not let it detach, which would prevent the following ro/cdrom flags from being set.
But, with a fresh boot, and currently no media persistence across reboots, the device should be created and empty, then emptied again, cdrom/ro flags set, and media inserted.

If you could confirm that /etc/init.d/S03usbdev is the new version (given its copied from kvmapp) i have been caught out by not updating it.

@scpcom
Copy link
Contributor

scpcom commented May 25, 2025

Yes, I can confirm S03usbdev is the new version.
I get no error messages in web interface, no error message in the logs.
I can see the cdrom flag is set if I do this:
cat /sys/kernel/config/usb_gadget/g0/functions/mass_storage.disk0/lun.0/cdrom
But it is not recognized by the Windows target.

The strange thing is: If I put the "Virtual Image" switch off and on again and CD-ROM mode works, I do not need to reset again. After this I can switch from/to CD-ROM mode without reset.
Maybe we remove the usb reset changes here and discuss this separately.

@tpretz
Copy link
Contributor Author

tpretz commented May 26, 2025

This makes a bit more sense, KVM is applying changes but running into host USB / OS behavioural differences.

How about this commit?

  • Restored default behaviour of resetting the USB gadget on media change
  • But, added a /boot/usb.norst flag, which I / others can set to further test on different hardware which disables the full reset
  • No GUI setting changes, this is, like many of the HID tweaks, a manually set flag most end-users won't care about

@scpcom
Copy link
Contributor

scpcom commented May 26, 2025

Yes, I think this i a good approach.

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.

2 participants