Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Using global static variables for per-device state breaks support for multiple connected devices and causes state collisions. - [High] Bypassing the Rust workqueue abstractions to call the raw C `cancel_work_sync` leaks the `Arc` reference transferred to the workqueue. - [Medium] The async IN queue for the EP84 status endpoint is incorrectly configured with an OUT endpoint address (`0x04`), causing the queue creation to fail. --
commit d6f1e7eb7413b3d79af598de7aa00c7613895253 Author: Mike Lothian <[email protected]> drm/vino: add the DisplayLink DL3 dock driver This commit adds a new open-source Rust driver for DisplayLink DL3 USB docks, including the Dell D6000. It introduces the USB bulk and control transfer handshake, HDCP 2.2 control plane, and registers a DRM/KMS device. > diff --git a/drivers/gpu/drm/vino/vino.rs b/drivers/gpu/drm/vino/vino.rs > new file mode 100644 > index 0000000000000..eb4378a747c35 > --- /dev/null > +++ b/drivers/gpu/drm/vino/vino.rs > @@ -0,0 +1,2617 @@ > +// SPDX-License-Identifier: GPL-2.0 [ ... ] > +/// Set once the dock has actually engaged the CP cipher (`wsub=0x45` acks > > 0). EP08 video is > +/// gated on it: pushing frames at a dock whose CP channel is dead makes it > fault and USB-reset. > +/// NOTE: with the current CP-engagement wall (see the file header) this is > never set on real > +/// hardware -- the dock runs the whole plaintext handshake but never > engages the encrypted CP. > +static CP_ENGAGED: core::sync::atomic::AtomicBool = > core::sync::atomic::AtomicBool::new(false); > + > +/// One-shot: clear-halt + prime the video endpoints before the first > live-scanout EP08 write. > +static EP08_SCANOUT_PRIMED: core::sync::atomic::AtomicBool = > + core::sync::atomic::AtomicBool::new(false); > + > +/// Select the live scanout codec. `false` (default) = the RLE/mode-2 path > (`video::Encoder`), > +/// which is what the dock currently runs and which the dock NAKs pre-CP > anyway. `true` = the > +/// byte-exact Vino WHT **colour** codec (`video::wht::colour_frame_ep08`), > the bandwidth-efficient > +/// DLM-quality path. Kept a compile-time flag (default off) so the RLE path > stays the live default > +/// and the switch is testable independently the moment the CP wall falls; > the WHT path needs a > +/// 64x16-aligned mode and falls back to RLE otherwise (see > `docs/VIDEO-TODO.md`). > +const EP08_WHT_CODEC: bool = false; > + > +/// Consecutive failed live-scanout frames, for log rate-limiting. Until CP > engages, the dock > +/// NAKs every EP08 write (EPROTO), so without this every compositor > pageflip would spam dmesg. > +static SCANOUT_FAILS: core::sync::atomic::AtomicU64 = > core::sync::atomic::AtomicU64::new(0); > + > +/// Pageflip throttle: number of upcoming pageflips to skip before the next > scanout attempt > +/// (a backoff while the dock NAKs). A single successful frame clears it. > +static SCANOUT_SKIP: core::sync::atomic::AtomicU64 = > core::sync::atomic::AtomicU64::new(0); > + > +/// Set once the bring-up work item finishes (AKE/CP attempt done). `detect` > only connects the > +/// live-scanout connector AFTER this, so a compositor enabling the output > cannot start EP08 > +/// scanout on top of the still-running AKE on the same USB device. > +static BRINGUP_COMPLETE: core::sync::atomic::AtomicBool = > + core::sync::atomic::AtomicBool::new(false); [Severity: High] Does this code support multiple connected devices? It looks like these global static variables are used to track per-device state. If multiple DisplayLink D6000 docks are connected, won't they race on these shared flags during initialization and operation? For instance, one device completing bring-up could incorrectly flag all devices as ready via BRINGUP_COMPLETE. [ ... ] > + // Open the persistent async EP84 IN reader BEFORE the arm marker > and msg0, so > + // `EP84_QUEUE_DEPTH` IN transfers are already posted when the dock > pushes its post-arm > + // reply (DLM's libusb always-pending-IN behaviour). Draining EP84 > concurrently stops the > + // dock's IN FIFO filling and NAKing our OUT (the sync-bulk deadlock > that produced a 100 ms > + // msg0 NAK). RAII: dropping the queue at function exit kills+frees > the URBs. > + let mut ep84_q = match dev.bulk_in_queue(0x04, EP84_QUEUE_DEPTH, > EP84_BUF) { [Severity: Medium] Is the endpoint address correct here in VinoDriver::send_cp_setup()? It looks like it's opening an IN queue for endpoint 0x84 but passes 0x04 instead. Since 0x04 is missing the USB_DIR_IN bit (0x80), won't the queue creation fail and force the driver to fall back to the synchronous bulk_recv mode? [ ... ] > + fn disconnect<'bound>(intf: &'bound usb::Interface<Core<'_>>, data: > Pin<&Self>) { > + let dev: &device::Device<Core<'_>> = intf.as_ref(); > + // Flush the deferred bring-up before the interface is unbound: > `cancel_work_sync` > + // dequeues it if pending and blocks until it returns if already > running, so no > + // USB I/O races the unbind (see the `Interface::as_bound` contract > in > + // `BringUp::run`). Safe to call when the work already finished or > never ran. > + if let Some(work) = data.bringup.as_ref() { > + // SAFETY: `work.work` is a live `Work` field of a pinned, > refcounted `BringUp`; > + // `raw_get` yields its valid `work_struct`, which > `cancel_work_sync` only reads > + // and synchronises against. The `Arc` keeps the allocation > alive across the call. > + unsafe { > + let wptr = Work::raw_get(core::ptr::addr_of!(work.work)); > + bindings::cancel_work_sync(wptr); > + } > + } [Severity: High] Does this direct call to bindings::cancel_work_sync() in VinoDriver::disconnect() leak the Arc reference transferred to the workqueue? Since the Rust workqueue enqueue consumes an Arc<BringUp> that is usually dropped when the callback executes, bypassing the Rust abstractions to cancel the work means the callback won't run, leaving the Arc without a mechanism to drop it. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
