Re: [RFC PATCH v2 06/10] drm/vino: add the DisplayLink DL3 dock driver

[sashiko-bot@kernel.org]

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 <mike@fireburn.co.uk>
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/20260703030217.2886-1-mike@fireburn.co.uk?part=6