Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider: - [High] Sleeping functions are called in the atomic commit path (`atomic_update`), causing "scheduling while atomic" panics. - [High] Global encode workqueue is never destroyed, leaking kernel threads on module unload. - [High] Permanent endpoint wedging due to unhandled queue transmission errors during activation and keepalive. - [High] Missing bracket unwind and `programmed_timing` cleanup on partial dual-wake failures. - [High] Race condition in workqueue completion prevents `ShadowSurface` recycling, causing massive per-frame memory reallocations. - [High] Global EDID target variables are mutated without synchronization, causing dropped monitors during concurrent modesets. - [High] Lockless checks in `activate_head` allow concurrent execution, leading to hardware sequence corruption and AES desynchronization. - [High] Global `video_staging` Mutex is held across synchronous USB operations, causing severe cross-monitor starvation. - [High] Incorrect bitmask check for reflection flags treats reflected modes as identity, corrupting display updates. - [Medium] Unhandled USB receive errors cause a 64ms CPU busy-wait loop in control message routines. --
commit 75ffbefbbe278fc4d8fd7640be258d86e92c774e Author: Mike Lothian <[email protected]> drm/vino: add the dock activation and scanout path Add the dock-facing half of the sink, bringing up a connector's downstream sink through a sequenced stream brackets, carrier frames, and mode sets. > diff --git a/drivers/gpu/drm/vino/drm_sink/activation.rs > b/drivers/gpu/drm/vino/drm_sink/activation.rs > new file mode 100644 > index 0000000000000..e505d80ed80d8 > --- /dev/null > +++ b/drivers/gpu/drm/vino/drm_sink/activation.rs [ ... ] > + self.send_stream_open(dev, connector_index)?; > + // A stream that is being opened here starts its ring and its > frame counter from the > + // beginning, whatever the connector reached before. Arming the > prologue already resets > + // this, but an activation that is retried arms once and > presents several times, so a > + // connector could open a stream and immediately tell the dock > it was filling a later > + // slot with a later frame number -- and the dock scans out a > slot nothing wrote. > + self.scanout_seq.lock()[connector_index] = 0; > + } > + let startup = arm.is_some(); > + let seq0 = self.scanout_seq.lock()[connector_index]; > + let started = Instant::<Monotonic>::now(); > + let mut repeat = 0u32; > + // Presentations that named a ring slot, which is what the frame > counter counts. See > + // `names_ring_slot`. > + let mut named = 0u32; [Severity: High] Does this code race to update the sequence numbers? If two threads concurrently execute submit_prompt_training() due to the lockless activate_head() invocation, won't they interleave CP control messages and perform concurrent overlapping read-modify-write operations on scanout_seq? [ ... ] > + let wire_len = arm_slice.len() > + + opener_slice.len() > + + report_slice.len() > + + params_slice.len() > + + image_len > + + trailer.len(); > + { > + // One writer owns a shared pipe for the whole frame; see > `own_pipe`. > + let _pipe = self.own_pipe(); > + let mut staging_slots = self.video_staging.lock(); > + let staging_slot = &mut staging_slots[connector_index]; > + if staging_slot.is_none() { > + let mut staging = KVec::new(); > + staging.resize(xfer, 0, GFP_KERNEL)?; > + *staging_slot = Some(staging); > + } > + let staging = > staging_slot.as_mut().ok_or(kernel::error::code::ENOMEM)?; > + > + let mut queue_slot = self.video_q[pipe_i].lock(); [Severity: High] Does holding this global Mutex stall all other monitors? In submit_prompt_training(), the global video_staging.lock() is acquired to obtain the staging buffer. But instead of extracting the buffer, the lock is held for the duration of the while wire_off < wire_len loop below. Since the loop synchronously calls queue.send() with a timeout, won't this indefinitely block any other connector attempting to render a frame in encode_and_send_haar() (which also requires video_staging.lock()) until this sequence finishes? [ ... ] > + // DLM's mixed transport: prologue chunk zero is reaped > below, then the rest > + // and all ordinary frames are pipelined. > + queue.send(dev.io(), dst, crate::timeout())?; [Severity: High] Could this leave the video queue stuck in an error state permanently? If queue.send() returns a transient USB transport error during submit_prompt_training(), the ? operator bubbles it up without calling retire_failed_video_queue(). Unlike the main encode_and_send_haar() scanout path, this fails to clear the USB halt condition, which wedges the endpoint for all subsequent frames. [ ... ] > + let (sent, started) = match timeline { > + Ok(state) => state, > + Err(e) => { > + for connector in 0..MAX_CONNECTORS { > + if sent & (1u32 << connector) != 0 > + && > self.modeset_active[connector].load(Ordering::Acquire) == keys[connector] > + { > + self.modeset_active[connector].store(0, > Ordering::Release); > + } > + } > + // The choreography opens every activating connector's > bracket well before it sets a > + // mode, so unwind on `valid` rather than `sent`: a > connector that failed before its > + // mode set is still open on the dock. > + for connector in 0..MAX_CONNECTORS { > + if valid & (1u32 << connector) != 0 { > + self.unwind_bracket(dev, connector as u8); > + } > + } > + return Err(e); [Severity: High] Does this error path fail to clean up programmed_timing? In activate_dual_wake(), when timeline returns an Err, the code correctly calls unwind_bracket() but forgets to clear programmed_timing before unwinding. Could this desynchronize the link state? > + } > + }; > + if sent.count_ones() < 2 { > + for connector in 0..MAX_CONNECTORS { > + if sent & (1u32 << connector) != 0 > + && > self.modeset_active[connector].load(Ordering::Acquire) == keys[connector] > + { > + self.modeset_active[connector].store(0, > Ordering::Release); > + } > + } > + return Ok(false); > + } [Severity: High] Is this missing a bracket unwind and programmed_timing cleanup? When sent.count_ones() < 2 (e.g. one connector failed earlier), the code resets modeset_active and returns Ok(false). However, unlike activate_dock_wide and the Err(e) branch above, it doesn't call unwind_bracket(dev, connector) or clear programmed_timing. Does this leave the dock hardware in a partially open bracket state? > diff --git a/drivers/gpu/drm/vino/drm_sink/cp_session.rs > b/drivers/gpu/drm/vino/drm_sink/cp_session.rs > new file mode 100644 > index 0000000000000..36b3a46026a55 > --- /dev/null > +++ b/drivers/gpu/drm/vino/drm_sink/cp_session.rs [ ... ] > + let mut reply = KVec::from_elem(0u8, 4096, GFP_KERNEL)?; > + let deadline = Instant::<Monotonic>::now() + Delta::from_millis(64); > + let mut matched = 0usize; > + let (mut reaped, mut undecodable) = (0u32, 0u32); > + let (mut seen_id, mut seen_sub, mut seen_counter) = (0u16, 0u16, > 0u16); > + loop { > + let got = if let Some(q) = link.ep84_q.as_mut() { > + match q.recv(dev.io(), &mut reply, > crate::cp_reply_timeout()) { > + Ok(Some(n)) => n, > + Ok(None) => 0, > + Err(_) => break, > + } > + } else { > + dev.ctrl_recv(&mut reply, crate::cp_reply_timeout(), > GFP_KERNEL) > + .unwrap_or(0) > + }; [Severity: Medium] Will this loop spin indefinitely on USB receive errors? In send_cp_reply(), if dev.ctrl_recv() encounters a transport error, it is masked by unwrap_or(0). This results in got = 0, which bypasses the break condition inside match q.recv and immediately loops again to evaluate the 64ms timeout deadline. Could this cause the kernel thread to busy-wait in a tight loop without yielding the CPU when the device stops responding? > diff --git a/drivers/gpu/drm/vino/drm_sink/presence.rs > b/drivers/gpu/drm/vino/drm_sink/presence.rs > new file mode 100644 > index 0000000000000..6e58f5729223c > --- /dev/null > +++ b/drivers/gpu/drm/vino/drm_sink/presence.rs [ ... ] > + pub(crate) fn reengage_connector( > + &self, > + io: &BoundInterface<'_>, > + connector: u8, > + ) -> Result<bool> { > + let socket = connector + 1; > + self.set_self_blanked(connector as usize, false); > + // A connector that is not answering is exactly a connector that may > be sitting in an open > + // bracket, where the dock has disengaged its EDID handler and every > probe below would go > + // unanswered. The state is the dock's and it survives a > re-enumeration, so a fresh session > + // cannot know it is owed: assert the closed state rather than infer > it. A connector already > + // closed ignores this. > + self.close_bracket_before_probe(io, connector); > + self.edid_target.store(connector as u32, Ordering::Release); > + *self.edid_caught.lock() = None; [Severity: High] Is it safe to mutate these global EDID variables without synchronization? In reengage_connector(), self.edid_target and self.edid_caught are modified. Since this is invoked from close_blank_bracket() during activate_head(), it can run concurrently for two independent connectors via separate scanout workers. If Thread B overwrites Thread A's edid_target, won't Thread A's incoming EDID response on the shared CP session be misattributed to Thread B, causing dropped monitors during concurrent modesets? [ ... ] > + let mut reply = KVec::from_elem(0u8, 4096, GFP_KERNEL).ok()?; > + let deadline = Instant::<Monotonic>::now() + Delta::from_millis(64); > + let got = loop { > + let n = match link.ep84_q.as_mut() { > + Some(q) => match q.recv(dev.io(), &mut reply, > crate::cp_reply_timeout()) { > + Ok(Some(n)) => n, > + Ok(None) => 0, > + Err(_) => return None, > + }, > + None => dev > + .ctrl_recv(&mut reply, crate::cp_reply_timeout(), > GFP_KERNEL) > + .unwrap_or(0), > + }; [Severity: Medium] Is it possible for this loop to busy-wait and flood the USB subsystem? Similarly to send_cp_reply(), if the fallback dev.ctrl_recv() is used in send_presence_probe(), any transport error is swallowed by unwrap_or(0). The loop will continue to spin until the 64ms deadline expires. > diff --git a/drivers/gpu/drm/vino/drm_sink/scanout.rs > b/drivers/gpu/drm/vino/drm_sink/scanout.rs > new file mode 100644 > index 0000000000000..2edd29cffaf6b > --- /dev/null > +++ b/drivers/gpu/drm/vino/drm_sink/scanout.rs [ ... ] > + data.last_frame.lock()[connector_index] = > Some(Instant::<Monotonic>::now()); > + let returned = Arc::into_unique_or_drop(src).map(|src| { > + let mut src = core::pin::Pin::into_inner(src); > + ShadowSurface { > + w: source_w, > + h: source_h, > + pixels: core::mem::replace(&mut src.pixels, KVVec::new()), > + hashes: core::mem::replace(&mut src.hashes, KVVec::new()), > + band, > + } > + }); [Severity: High] Does this fail to recycle the ShadowSurface buffer pool? In run_pending_scanout(), Arc::into_unique_or_drop(src) expects a strong_count of 1. But because of the race condition in EncodeChunk::run(), the background thread might still hold its reference. If into_unique_or_drop() fails, the large memory buffers (up to 15MB) are dropped instead of returning to the pool. Could this cause heavy continuous GFP_KERNEL churn and potential OOMs? [ ... ] > +pub(super) fn snapshot_to_shadow( > + geometry: crate::video::haar::Geometry, > + slot: &mut Option<ShadowSurface>, > + source: &kms::framebuffer::FramebufferVMapOwned<VinoObject>, > + w: usize, > + h: usize, > +) -> Result { [ ... ] > + let mut fresh = false; > + if !matches!(slot, Some(s) if s.w == w && s.h == h) { > + let mut pixels: KVVec<u8> = KVVec::new(); > + pixels.resize(need, 0, GFP_KERNEL)?; > + let mut hashes: KVVec<u64> = KVVec::new(); > + hashes.resize(tiles_x * tiles_y, 0, GFP_KERNEL)?; > + let mut band: KVVec<u8> = KVVec::new(); > + band.resize(band_len, 0, GFP_KERNEL)?; [Severity: High] Can this sequence trigger a scheduling-while-atomic panic? When VinoPlane::atomic_update executes data.queue_scanout(), it synchronously calls snapshot_to_shadow() from within the atomic context. Here, snapshot_to_shadow() dynamically allocates large KVVec buffers using GFP_KERNEL, which is a sleeping allocation. Does this break the DRM subsystem requirement that atomic commit paths must not sleep? [ ... ] > +pub(super) fn read_cursor_bgra( > + fb: &kms::framebuffer::Framebuffer<VinoDrmDriver>, > + w: usize, > + h: usize, > +) -> Result<KVec<u8>> { > + let vmap = fb.vmap::<VinoObject>()?; > + let view = vmap.view(); > + let pitch = vmap.pitch(); > + let row = w.checked_mul(4).ok_or(EINVAL)?; > + let len = row.checked_mul(h).ok_or(EINVAL)?; > + let mut out = KVec::new(); > + out.resize(len, 0, GFP_KERNEL)?; [Severity: High] Does this code call a sleeping function inside an atomic context? In VinoPlane::atomic_update, the atomic commit path acquires a sleeping Mutex (plane.cursor_last.lock()) and calls read_cursor_bgra(). Here in read_cursor_bgra(), it allocates with GFP_KERNEL. Since the DRM subsystem forbids sleeping inside atomic_update callbacks, could this cause deadlocks or scheduling-while-atomic panics during cursor updates? [ ... ] > +fn encode_queue() -> Option<&'static workqueue::Queue> { > + static ENCODE_WQ: kernel::sync::SetOnce<workqueue::OwnedQueue> = > kernel::sync::SetOnce::new(); > + if let Some(q) = ENCODE_WQ.as_ref() { > + return Some(q); > + } [Severity: High] Does this code leak the workqueue threads when the module is unloaded? ENCODE_WQ is statically allocated using kernel::sync::SetOnce. In Rust, static variables do not automatically run Drop destructors. Without a module-level teardown mechanism, doesn't the unbound workqueue remain in the system forever, permanently leaking resources? [ ... ] > +impl WorkItem for EncodeChunk { > + type Pointer = Arc<EncodeChunk>; > + > + fn run(this: Arc<EncodeChunk>) { > + if let Ok(strips) = encode_coords(this.geometry, &this.src, > &this.coords) { > + *this.out.lock() = strips; > + } > + // Complete unconditionally. On failure `out` stays short and the > joiner detects that by > + // length -- but it must never be left blocked on a completion that > cannot fire. > + this.done.complete_all(); > + } > +} [Severity: High] Can this complete_all() call race with the scanout thread's Arc recycling? If this.done.complete_all() is signaled before the background workqueue thread returns and drops its Arc<EncodeChunk> reference, the waiting scanout thread might wake up immediately. [ ... ] > + if > data.modeset_active[connector_index].load(core::sync::atomic::Ordering::Acquire) > != want { > + // A failed command-worker activation leaves the desired generation > intact. This worker is > + // sleepable, so retry the same transaction before submitting its > pending framebuffer. > + let timing = cached.ok_or(EINVAL)?; > + data.activate_head(dev, connector, &timing, want)?; [Severity: High] Can this lockless check cause concurrent hardware programming? If the command worker processes KmsCmd::ModeSet and calls activate_head(), and the deferred scanout worker evaluates this lockless modeset_active != want check, both threads might enter activate_head() simultaneously. Without a per-connector lock preventing both from executing the hardware transaction, won't this result in scrambled hardware programming and AES decryption failures? [ ... ] > + // Full keyframe vs damage delta. A mode-set requires a keyframe; > rotation/reflection remains > + // conservative because the content shadow is deliberately stored in > unrotated framebuffer > + // space. For identity rotation, compare the actual framebuffer instead > of trusting optional > + // FB_DAMAGE_CLIPS: KWin commonly changes framebuffer objects without > publishing that blob. > + let kf_bit = 1u32 << connector_index; > + let identity = rotation.angle() == plane::Rotation::ROTATE_0 > + && !rotation.contains(plane::Rotation::REFLECT_X | > plane::Rotation::REFLECT_Y); [Severity: High] Is this bitmask check for reflection flags correct? In encode_and_send_haar(), the identity fast-path evaluates !rotation.contains(plane::Rotation::REFLECT_X | plane::Rotation::REFLECT_Y). Because contains() returns true only if ALL specified bits are present, a plane with only REFLECT_X or only REFLECT_Y evaluates to false, making identity = true. Doesn't this bypass the conservative full-frame update for single-axis reflections, passing unreflected framebuffer damage coordinates to damage_strip_coords() and corrupting the display updates? > diff --git a/drivers/gpu/drm/vino/drm_sink/stream.rs > b/drivers/gpu/drm/vino/drm_sink/stream.rs > new file mode 100644 > index 0000000000000..1adb37de980e0 > --- /dev/null > +++ b/drivers/gpu/drm/vino/drm_sink/stream.rs [ ... ] > + pub(super) fn send_stream_open(&self, dev: &BoundInterface<'_>, > connector: usize) -> Result { > + let bit = 1u32 << connector; > + if self.stream_open_pending.load(Ordering::Acquire) & bit == 0 { > + return Ok(()); > + } > + let Some(open) = self.build_stream_open_buf(connector)? else { > + self.stream_open_pending.fetch_and(!bit, Ordering::Release); > + return Ok(()); > + }; > + let pipe_i = dev.video_pipe_index(connector)?; > + let mut queue_slot = self.video_q[pipe_i].lock(); > + if queue_slot.is_none() { > + *queue_slot = Some(dev.video_queue(connector, 8, VIDEO_XFER)?); > + } > + let queue = queue_slot > + .as_mut() > + .get_mut() > + .as_mut() > + .ok_or(kernel::error::code::ENODEV)?; > + queue.send(dev.io(), &open, crate::timeout())?; [Severity: High] Does this error path also skip retiring the failed video queue? If queue.send() fails in send_stream_open(), the error is propagated without resetting the queue state, leading to the same permanent endpoint wedge mentioned above. [ ... ] > + pub(crate) fn send_video_keepalive(&self, dev: &BoundInterface<'_>) { > + if self.uses_arm_burst() || > self.shutting_down.load(Ordering::Acquire) { > + return; > + } [ ... ] > + let Some(queue) = queue_slot.as_mut().get_mut().as_mut() else { > + continue; > + }; > + if queue.send(dev.io(), &report, crate::timeout()).is_ok() { > + self.last_video_at.lock()[connector] = > Some(Instant::<Monotonic>::now()); > + } > + } > + } [Severity: High] Is it safe to ignore queue.send() errors here? By ignoring the error in send_video_keepalive(), the broken queue state is left active. When the next foreground frame tries to use this queue, it will fail since retire_failed_video_queue() was never invoked. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
