Two parallel review agents cross-checked the virtio-inputd driver against
the Linux 7.1 reference (drivers/virtio/virtio_input.c, virtio_input.h)
and the proven redox-drm virtio transport. 12 issues were found across
BLOCKER, MAJOR, MINOR, and NIT severity, all fixed in this commit before
runtime testing.
BLOCKERs (would have prevented the driver from working in QEMU):
1. fill_avail() never wrote avail_idx after pushing the 64 ring
entries. The device reads avail_ring[avail_idx % size] to discover
new buffers, so without publishing avail_idx = size, the device
saw avail_idx = 0 and ignored all initial buffers. Fix: explicit
'fence(Release); write_avail_idx(self.size)' with a spec citation.
2. drain() recycled IDs derived from 'last_used_idx - drained_count',
which is wrong when the used ring wraps and a single drain cycle
spans more than one full ring revolution. Fix: collect the actual
drained 'id' values in a stack '[u16; 64]' array during the drain
loop, then push those exact ids back to the avail ring. The
doc-comment explains why the derivation is unsafe.
3. config_read_string() and config_read_bitmap() used
'self.device_cfg.size()' (the MMIO region size = 40) instead of
the device-reported config size from offset 2. Fix: use
'config_read_size()' to read the actual size field.
4. config/redbear-full.toml: device_id_range was a TOML string
('0x1042..=0x107F') but serde's Range<u16> deserializes from a
sequence, not a string. pcid-spawner would have silently failed
to load the fragment. Fix: use serde array form
'device_id_range = [0x1042, 0x107F]'.
MAJORs (silent failure modes or runtime bugs):
5. activate_queue() had no fence between address writes and
'queue_enable = 1'. A CPU write buffer may reorder writes to
distinct MMIO addresses. Fix: explicit 'fence(SeqCst)' with
a comment citing virtio spec 2.8 and Linux's virtio_wmb.
6. No 'reset_device()' on error path after partial init. The device
would be left in ACKNOWLEDGE|DRIVER with no driver active,
requiring a guest reboot to recover. Fix: wrap init in a closure;
any error calls 'transport.reset_device()' before propagating.
7. Drain loop never checked DEVICE_NEEDS_RESET or DEVICE_STATUS_FAILED.
If the device entered an unrecoverable state, the driver would
poll forever with stale state. Fix: 'device_in_error_state()' on
the transport; loop checks it each iteration and exits cleanly.
8. The abs_count probe used 'config_read_size() == 24', which was
always false (virtio_input_absinfo is 20 bytes, not 24). The
count was always logged as 0. Fix: '>= 20' per spec.
MINORs / NITs (hardening, no functional impact):
9. config_read_absinfo() returned AbsInfo without validating
device-reported size. Now returns Option<AbsInfo> and validates
size >= 20.
10. map_cap_region() missing bounds check: capability range may
extend past BAR end (QEMU is permissive; bare-metal is not).
Added 'cap_end > bar_size' check with spec reference.
11. Legacy device 0x1052 entry in pcid fragment caused spurious
spawn + log noise. Removed.
12. notify_queue() error silently dropped with .ok(). Now logs warn
and continues.
Plan update:
The CONSOLE-TO-KDE-DESKTOP-PLAN.md v5.1 changelog now has a new
'9.1.1 Phase 5.1 review-driven fixes' section documenting all 12
findings with file:line, severity, and fix. Future maintainers can
trace the BLOCKERs back to specific commits to understand the
critical-path safety net this review provided.
Verification: cargo check zero errors, 64 warnings (all unused
keycode constants reserved for Phase 5.2 expansion). The driver
is now ready for runtime testing in QEMU.
vasilito
b681a2fb66