[RFC PATCH v4 0/3] rust: Add drm::JobQueue

[RFC PATCH v4 0/3] rust: Add drm::JobQueue

[Philipp Stanner]

As the commit messages and code comments detail, progressing JobQueue is
currently somewhat blocked because of an issue with self-referential
PinInit, which Gary generously offered to investigate.

This code compiles, but the example does not because of the
aformentioned issue. Nevertheless, I wanted to provide another RFC here
so that we can move our discussions forward in the meantime, especially
since very much about JobQueue has changed.

Our, now upstreamed, DmaFence abstractions informed some of the notable
changes in JobQueue. Most notably, JobQueue now owns the FenceContext,
Jobs are created on the queue and own the DriverFence. Jobs, again, are
owned by the JobQueue. We hope to enforce correct behavior that way,
having FenceContext, JobQueue and firmware ring all correspond with each
other 1:1:1.

I suspect that a potential deadlock on JQ drop still exists. I
previously solved that with Revocable, which I tend to think will also
be the way to go here.

(One very great news btw is that we almost magically solved a number of
issues with the new DmaFence design – in combination with this JobQueue
design, for the first time it would be possible to fully support the
dma_fence backend_ops. The driver-unload-problem previously had most
users use intermediate fences, like the drm_sched_fence, which means
that callbacks, e.g. from userspace, could not be passed through to the
driver. Now, with FenceContextOps <-> JobQueueOps, we can theoretically
support all of them)

The differences between this draft and Daniel / Tyr's prototype which
probably are most noteworthy are the different lock design
("Philipp"-JobQueue has one big lock, wheres Tyr-JobQueue has 2, 3 if
you count the XArray lock) and the used data structure.

The presented solution uses lists over XArray because:
  1. XArray is semantically more complex and has an additional lock.
  2. An Xarray-as-ringbuffer needs own algorithms for index tracking,
     wrapp around etc. With list you enqueue into a waiting list, and
     for running you move into the running_list.
  3. The memory reservations we need for pre-allocating everything for
     our job-submission path are solved in one go with list, because a
     job simply contains a list head.
  4. Most notably, it is unclear how XArray behaves for ever-increasing
     indices with a sliding window, whereas the list semantic is well
     understood and deterministic.

I obviously don't claim to own all the wisdom in that regard; it's just
that I still propose this solution because these arguments make me
believe that it is the right one.

Since all the list handling is done with iterators, there is currently
no unsafe needed.


I hope we can discuss many things here before we, hopefully soon, can
address the lifetime issue and move to a v1.


This is based on drm-rust-next (10a6623a24a8), plus Danilo's ScopedWork
[1] and my patches [2][3] regarding 'static and lock errors.

Regards,
Philipp

[1] https://lore.kernel.org/rust-for-linux/20260807165252.3849875-1-dakr@kernel.org/
[2] https://lore.kernel.org/rust-for-linux/20260922083631.444614-2-phasta@kernel.org/
[3] https://lore.kernel.org/rust-for-linux/20260924085523.2620704-2-phasta@kernel.org/

Philipp Stanner (3):
  rust: DmaFence: remove static lifetime
  rust: DmaFence: Implement Deref for FenceContext
  rust: drm: Add JobQueue

 rust/kernel/dma_buf/dma_fence.rs |  17 +-
 rust/kernel/drm/job_queue.rs     | 493 +++++++++++++++++++++++++++++++
 rust/kernel/drm/mod.rs           |   4 +
 3 files changed, 510 insertions(+), 4 deletions(-)
 create mode 100644 rust/kernel/drm/job_queue.rs

-- 
2.55.0

[RFC PATCH v4 1/3] rust: DmaFence: remove static lifetime

[Philipp Stanner]

(will be upstreamed separately)

Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 rust/kernel/dma_buf/dma_fence.rs | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
index 18a43e1bb442..eef2b9e93f35 100644
--- a/rust/kernel/dma_buf/dma_fence.rs
+++ b/rust/kernel/dma_buf/dma_fence.rs
@@ -291,7 +291,7 @@ fn from(e: AllocError) -> Self {
 ///     }
 /// }
 /// ```
-pub trait FenceCallback: Send + 'static {
+pub trait FenceCallback: Send {
     /// Called when the fence is signaled.
     ///
     /// This is called from the fence signaling path, which may be in interrupt
@@ -310,7 +310,7 @@ pub trait FenceCallback: Send + 'static {
 /// When this object is dropped, the callback is automatically removed if it
 /// hasn't been called yet.
 #[pin_data(PinnedDrop)]
-pub struct FenceCallbackRegistration<T: FenceCallback + 'static> {
+pub struct FenceCallbackRegistration<T: FenceCallback> {
     #[pin]
     callback_foreign: Opaque<bindings::dma_fence_cb>,
     callback: ManuallyDrop<T>,
@@ -326,7 +326,7 @@ impl<T: FenceCallback> FenceCallbackRegistration<T> {
     /// On success the callback is pinned in place and will fire when the fence
     /// signals. On `AlreadySignaled` the callback is returned to the caller so
     /// that owned resources can be reclaimed.
-    pub fn new<'a>(fence: &'a Fence, callback: T) -> impl PinInit<Self, CallbackError<T>> + 'a
+    pub unsafe fn new<'a>(fence: &'a Fence, callback: T) -> impl PinInit<Self, CallbackError<T>> + 'a
     where
         T: 'a,
     {
@@ -693,7 +693,8 @@ struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> {
 ///
 /// let cb_data = CallbackData { };
 /// let waiting_fence = ARef::from(fence.as_fence());
-/// let cb_reg = FenceCallbackRegistration::new(&waiting_fence, cb_data);
+/// // SAFETY: `cb_data`'s content is not forgotten.
+/// let cb_reg = unsafe { FenceCallbackRegistration::new(&waiting_fence, cb_data) };
 /// let cb_reg = KBox::pin_init(cb_reg, GFP_KERNEL)?;
 ///
 /// // TODO signalling guards
-- 
2.55.0

Re: [RFC PATCH v4 1/3] rust: DmaFence: remove static lifetime

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] rust: dma_fence: missing safety doc for FenceCallbackRegistration::new()
--

commit 6cb68d1acd60ccf038d3c22a96a083dfc30b1a48
Author: Philipp Stanner <phasta@kernel.org>

rust: DmaFence: remove static lifetime

This commit removes the static lifetime requirement for FenceCallback,
allowing callbacks to hold non-static references. It also updates
FenceCallbackRegistration::new to be unsafe to account for this change.

> diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
> index 18a43e1bb442f..eef2b9e93f356 100644
> --- a/rust/kernel/dma_buf/dma_fence.rs
> +++ b/rust/kernel/dma_buf/dma_fence.rs

[ ... ]

> @@ -326,7 +326,7 @@ impl<T: FenceCallback> FenceCallbackRegistration<T> {
>      /// On success the callback is pinned in place and will fire when the fence
>      /// signals. On `AlreadySignaled` the callback is returned to the caller so
>      /// that owned resources can be reclaimed.
> -    pub fn new<'a>(fence: &'a Fence, callback: T) -> impl PinInit<Self, CallbackError<T>> + 'a
> +    pub unsafe fn new<'a>(fence: &'a Fence, callback: T) -> impl PinInit<Self, CallbackError<T>> + 'a

[Severity: Medium]
Are we missing the safety documentation for this newly marked unsafe function?

If a developer calls FenceCallbackRegistration::new with a callback holding
local stack references and leaks the registration, could this result in a
use-after-free when the C backend eventually invokes the callback?

Should there be an explicit safety contract explaining this anti-leak
precondition to prevent bypassing memory safety guarantees?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929104703.934128-2-phasta@kernel.org?part=1

[RFC PATCH v4 2/3] rust: DmaFence: Implement Deref for FenceContext

[Philipp Stanner]

A FenceContext carries data, which the user should be able to access.

Implement Deref for FenceContext.

Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 rust/kernel/dma_buf/dma_fence.rs | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
index eef2b9e93f35..0028124e6aa6 100644
--- a/rust/kernel/dma_buf/dma_fence.rs
+++ b/rust/kernel/dma_buf/dma_fence.rs
@@ -86,6 +86,14 @@ pub struct FenceContext<T: FenceContextOps + Send + Sync> {
     data: T,
 }
 
+impl<T: Send + Sync + FenceContextOps> Deref for FenceContext<T> {
+    type Target = T;
+
+    fn deref(&self) -> &Self::Target {
+        &self.data
+    }
+}
+
 impl<'a, T: Send + Sync + FenceContextOps> FenceContext<T> {
     // This can later be extended as a vtable in case other parties need support
     // for the more "exotic" callbacks.
-- 
2.55.0

Re: [RFC PATCH v4 2/3] rust: DmaFence: Implement Deref for FenceContext

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] rust: dma_fence: missing #[inline] on FenceContext::deref
--

commit 4617358e176ca58ef1fbb67c98355887062d6801
Author: Philipp Stanner <phasta@kernel.org>

rust: DmaFence: Implement Deref for FenceContext

This commit implements the Deref trait for FenceContext, allowing users
to directly access the underlying data field.

> diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
> index eef2b9e93f356..0028124e6aa64 100644
> --- a/rust/kernel/dma_buf/dma_fence.rs
> +++ b/rust/kernel/dma_buf/dma_fence.rs
[ ... ]
> @@ -86,6 +86,14 @@ pub struct FenceContext<T: FenceContextOps + Send + Sync> {
>      data: T,
>  }
>  
> +impl<T: Send + Sync + FenceContextOps> Deref for FenceContext<T> {
> +    type Target = T;
> +
> +    fn deref(&self) -> &Self::Target {

[Severity: Low]
Should this method be annotated with #[inline]?

According to the Rust subsystem guidelines, small functions in abstraction
crates like the kernel crate should be annotated with the #[inline] attribute
to ensure optimal inlining across crate boundaries. The newly added deref
method is a trivial one-line accessor but lacks this attribute.

> +        &self.data
> +    }
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929104703.934128-2-phasta@kernel.org?part=2

[RFC PATCH v4 3/3] rust: drm: Add JobQueue

[Philipp Stanner]

JobQueue is a pure Rust component that handles job submissions for GPUs
with firmware scheduling, i.e., GPUs that support one exclusive ring per
execution context.

JobQueue's primary task is dependency handling, where the user can set
dependencies, each in the form of a `dma_buf::Fence`, on a job and then
submit it to the queue.

Jobs are being executed in order, following the FIFO principle. Only
jobs whose dependencies are all fullfilled get executed, otherwise the
JobQueue pauses.

A core design principle, just like with DmaFence, is to avoid all
allocations in job submission paths, since this runs danger of
deadlocking.

Another design principle, new when compared to the older drafts, is the
ownership relationship with fences:

  1. The JobQueue owns the FenceContext exclusively.
  2. Jobs are created on the JobQueue, ensuring correct fence sequence
     number ordering.
  3. Jobs own the respective DriverFence.
  4. Jobs are owned by the JobQueue.

Consequently, a driver can only interact with fences and FenceContext
indirectly through the JobQueue, and can also only complete jobs (and,
hence, signal fences) through the JobQueue.

Unfortunately, since jobs need a reference to the JobQueue for their
fence dependency objects (DependencyWaker), this code can currently not
be used in this form because some work on self-referential PinInit is
required. See the respective comments in the Example.

When a job shall be run, both its JobQueue::data and Job::data get
borrowed back to it, together with the sequence number of this job, so
that the driver can perform the appropriate interaction with the
hardware.

Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 rust/kernel/drm/job_queue.rs | 493 +++++++++++++++++++++++++++++++++++
 rust/kernel/drm/mod.rs       |   4 +
 2 files changed, 497 insertions(+)
 create mode 100644 rust/kernel/drm/job_queue.rs

diff --git a/rust/kernel/drm/job_queue.rs b/rust/kernel/drm/job_queue.rs
new file mode 100644
index 000000000000..9a6bc1e8fffc
--- /dev/null
+++ b/rust/kernel/drm/job_queue.rs
@@ -0,0 +1,493 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2026 Red Hat Inc.
+ * Author: Philipp Stanner <pstanner@redhat.com>
+ */
+
+//! DRM's job dependency manager and load balancer for GPUs with firmware
+//! scheduling, i.e., GPUs that spawn one ring per user context.
+//!
+//! JobQueue abstracts the entire handling of [`dma_buf::FenceContext`] and its
+//! associated fences for the user.
+//!
+//! Jobs created on the JobQueue take over ownership of userspace provided data
+//! and prepare all memory allocations. A later job submission sets up the fence
+//! and performs no fallible operations. A fence sequence number always
+//! corresponds 1:1 with the job sequence number.
+//!
+//! Before jobs are submitted, dependencies in the form of [`dma_buf::Fence`]
+//! can be set on them. Jobs get submitted by the JobQueue in process context
+//! through a work item in FIFO order. Only jobs whose dependency fences have
+//! all been signaled get submitted. If the front job's dependency is not yet
+//! fullfilled, the JobQueue pauses until that's the case.
+//!
+//! The driver tells the JobQueue which jobs are finished through
+//! [`JobQueue::complete_jobs_up_to_seqno`].
+//!
+//! This is pure Rust code that does not directly abstract C.
+
+use crate::prelude::*;
+
+use core::{
+    convert::Infallible,
+    ops::Deref, //
+};
+
+use kernel::{
+    dma_buf::dma_fence::{
+        DriverFence,
+        DriverFenceAllocation,
+        Fence,
+        FenceContext,
+        FenceCallback,
+        FenceCallbackRegistration,
+        FenceContextOps, //
+    },
+    list::*,
+    str::CString,
+    sync::{
+        aref::ARef,
+        SpinLockIrq,
+        new_spinlock_irq,
+        atomic::{
+            Atomic,
+            Relaxed, //
+        }, //
+    }, //
+    workqueue::{
+        self,
+        new_scoped_work,
+//        ScopedQueue,
+        ScopedWork,
+        ScopedWorkItem,
+        ScopedWorkRef,
+    }, //
+};
+
+struct DependencyWaker<'b, T: JobQueueOps + Send + Sync> {
+    nr_of_deps: &'b Atomic<u32>,
+    jq: &'b JobQueue<'b, T>,
+}
+
+impl<'b, T: JobQueueOps + Send + Sync> FenceCallback for DependencyWaker<'b, T> {
+    fn on_signal(&mut self) {
+        // Last dependency kicks off the worker.
+        if self.nr_of_deps.fetch_sub(1, Relaxed) == 1 {
+            // FIXME:
+            // I suspect that this is still the same deadlock-danger versus JQ's
+            // drop() that we solved previously with Revocable.
+            //
+            // If JQ drops, it takes the JQ-lock to deregister its waker callbacks
+            // on all external dependency Fences, thereby taking the fence lock.
+            // If one of those fences signal at that moment, first the fence lock
+            // will be taken, and then the JQ lock here -> deadlock.
+            //
+            // So we likely want to add the Revocable-solution from the first
+            // drafts again.
+            let mut jq = self.jq.inner.lock();
+            jq.as_mut().check_start_submit_worker();
+        }
+    }
+}
+
+/// A JobQueue job before it has been submitted.
+///
+/// This is only useful for registering dependencies on and for submitting it.
+pub struct Job<'a, T: JobQueueOps + Send + Sync> {
+    inner: ListArc<JobInternal<'a, T>>,
+}
+
+impl<'a, T: JobQueueOps + Send + Sync> Job<'a, T> {
+    /// Add `fence` as a dependency for this job.
+    ///
+    /// A JobQueue will only push a job once all dependency fences have been
+    /// signaled.
+    pub fn add_dependency(&'a mut self, fence: &Fence) -> Result {
+        let nr_of_deps = &raw const self.inner.nr_of_deps;
+
+        let waker = DependencyWaker {
+            // SAFETY: `nr_of_deps` lives as long as the job.
+            // TODO RFC: Is there a better way to do it? The mutable-immutable references
+            // otherwise run into conflict when `nr_of_deps` is used below.
+            nr_of_deps: unsafe { &*nr_of_deps },
+            jq: self.inner.jq,
+        };
+
+        let job = &mut self.inner;
+
+        job.dependencies().reserve(1, GFP_KERNEL)?;
+
+        // If the registration below gets registered successfully, a signaling
+        // fence will decrement the atomic. That could race. Thus, increment in
+        // advance. Ordering is ensured thanks to the dma_fence backend.
+        job.nr_of_deps.fetch_add(1, Relaxed);
+
+        // SAFETY: The `waker` is not forgotten.
+        let registration = unsafe { FenceCallbackRegistration::new(fence, waker) };
+        match KBox::pin_init(registration, GFP_KERNEL) {
+            Err(e) => {
+                let _ = job.nr_of_deps.fetch_sub(1, Relaxed);
+                return Err(e);
+            },
+            Ok(dep) => {
+                if let Err(e) = job.dependencies().push_within_capacity(dep) {
+                    // Impossible because of the reservation.
+                    pr_err!("Job::add_dependency(): reserved memory not available.\n");
+                    return Err(e.into());
+                }
+            },
+        }
+
+        Ok(())
+    }
+}
+
+#[pin_data]
+#[repr(C)]
+struct JobInternal<'b, T: JobQueueOps + Send + Sync> {
+    #[pin]
+    links: ListLinks,
+    fence_allocation: ListArcField<Option<DriverFenceAllocation<'b, DriverDataWrapper<T>>>, 0>,
+    fence: ListArcField<Option<DriverFence<'b, DriverDataWrapper<T>>>>,
+    dependencies: ListArcField<KVec<Pin<KBox<FenceCallbackRegistration<DependencyWaker<'b, T>>>>>>,
+    nr_of_deps: Atomic<u32>,
+    jq: &'b JobQueue<'b, T>,
+}
+
+impl_list_arc_safe! {
+    impl{'b, T: JobQueueOps + Send + Sync} ListArcSafe<0> for JobInternal<'b, T> { untracked; }
+}
+impl_list_item! {
+    impl{'b, T: JobQueueOps + Send + Sync} ListItem<0> for JobInternal<'b, T> { using ListLinks { self.links }; }
+}
+
+
+impl<'b, T: JobQueueOps + Send + Sync> JobInternal<'b, T> {
+//    const LIST_JOB_ID: u64 = 0xe7e445f2bb9e97fc;
+
+    kernel::list::define_list_arc_field_getter! {
+        pub(crate) fn fence_allocation(&mut self<0>) -> &mut Option<DriverFenceAllocation<'b, DriverDataWrapper<T>>> { fence_allocation }
+        pub(crate) fn fence(&mut self<0>) -> &mut Option<DriverFence<'b, DriverDataWrapper<T>>> { fence }
+        pub(crate) fn dependencies(&mut self<0>) -> &mut KVec<Pin<KBox<FenceCallbackRegistration<DependencyWaker<'b, T>>>>> { dependencies }
+    }
+}
+
+#[pin_data]
+struct SubmitWorker<'a, T: JobQueueOps + Send + Sync> {
+    #[pin]
+    jq: &'a JobQueue<'a, T>,
+}
+
+impl<T: JobQueueOps + Send + Sync> ScopedWorkItem for SubmitWorker<'_, T> {
+    fn run(work: &ScopedWorkRef<Self>) {
+        let fctx_data = work.jq.fctx.deref().deref();
+        let mut jq = work.jq.inner.lock();
+        let mut jq = jq.as_mut().project();
+
+        let mut cursor = jq.waiting_jobs.cursor_front();
+
+        while let Some(job) = cursor.peek_next() {
+            if job.nr_of_deps.load(Relaxed) > 0 {
+                break;
+            }
+
+            let mut job = job.remove();
+            let runnable_job = JobRunnable {
+                seqno: job.fence().as_ref().expect("fence not present").as_fence().seqno(),
+                data: job.fence().as_ref().expect("fence not present").deref(),
+            };
+
+            if !T::run_job(fctx_data, runnable_job) {
+                // There was no capacity left. The driver will invoke the JobQueue
+                // later again.
+                jq.waiting_jobs.push_front(job);
+                break;
+            }
+
+            jq.running_jobs.push_back(job);
+        }
+
+        *jq.submit_worker_active = false;
+    }
+}
+
+#[pin_data]
+struct JobQueueInner<'a, T: JobQueueOps + Send + Sync> {
+    submit_worker_active: bool,
+    #[pin]
+    submit_worker: ScopedWork<SubmitWorker<'a, T>>,
+    #[pin]
+    waiting_jobs: List<JobInternal<'a, T>>,
+    #[pin]
+    running_jobs: List<JobInternal<'a, T>>,
+}
+
+impl<T: JobQueueOps + Send + Sync> JobQueueInner<'_, T> {
+    fn check_start_submit_worker(self: Pin<&mut Self>) {
+        if self.submit_worker_active {
+            return;
+        }
+
+        // TODO this shouldn't be the system wq
+        // SAFETY: The worker is carried by the JobQueue and, thus, cannot be
+        // forgotten.
+        unsafe { workqueue::system_dfl().enqueue_scoped(&*self.submit_worker) };
+
+        *self.project().submit_worker_active = true;
+    }
+
+}
+
+/// A JobQueue.
+///
+/// # Examples
+///
+/// ```
+/// use core::ops::Deref;
+///
+/// use kernel::{
+///     str::CString,
+///     dma_buf::*,
+///     drm::{
+///         JobQueue,
+///         JobQueueOps,
+///         JobRunnable,
+///     },
+/// };
+///
+/// let driver_name = CString::try_from_fmt(fmt!("dummy_driver"))?;
+/// let timeline_name = CString::try_from_fmt(fmt!("dummy_timeline"))?;
+///
+/// #[pin_data]
+/// struct JqData {}
+///
+/// impl JqData {
+///     fn new() -> impl PinInit<Self> {
+///         pin_init!(Self {})
+///     }
+/// }
+///
+/// struct JobData {
+///     data: CString,
+/// }
+///
+/// impl JobQueueOps for JqData {
+///     type JobDataType = JobData;
+///
+///     fn run_job<'a>(&self, job: JobRunnable<'a, JobData>) -> bool {
+///
+///         true
+///     }
+/// }
+///
+/// let jq_data = JqData::new();
+/// let jq = KBox::pin_init(JobQueue::new(0, driver_name, timeline_name, jq_data), GFP_KERNEL)?;
+///
+/// struct Callback {}
+///
+/// impl FenceCallback for Callback {
+///     fn on_signal(&mut self) {
+///
+///     }
+/// }
+///
+/// let data = CString::try_from_fmt(fmt!("hello there!"))?;
+/// let data = JobData { data };
+///
+/// // TODO RFC:
+/// // This is the problem where self-references come into play.
+/// // Jobs need a reference to the JobQueue due to their DependencyWaker
+/// // callbacks. At the same time, the JobQueue owns the jobs.
+/// // So the following outcommented code would not compile.
+/// // Gary is working on a solution for that.
+///
+/// //let job = jq.new_job(data)?;
+/// //jq.submit(job);
+///
+/// //drop(jq);
+///
+/// Ok::<(), Error>(())
+/// ```
+#[pin_data(PinnedDrop)]
+pub struct JobQueue<'a, T: JobQueueOps + Send + Sync> {
+    #[pin]
+    inner: SpinLockIrq<JobQueueInner<'a, T>>,
+    #[pin]
+    fctx: FenceContext<DriverDataWrapper<T>>,
+}
+
+// SAFETY: The JobQueue is locked and pinned.
+unsafe impl<T: JobQueueOps + Send + Sync> Sync for JobQueue<'_, T> {}
+// SAFETY: The JobQueue is locked and pinned.
+unsafe impl<T: JobQueueOps + Send + Sync> Send for JobQueue<'_, T> {}
+
+/// Object passed to the driver to run a job.
+pub struct JobRunnable<'a, T> {
+    /// The job's sequence number.
+    pub seqno: u64,
+    /// The associated job data.
+    pub data: &'a T,
+}
+
+/// Ops for the JobQueue.
+pub trait JobQueueOps {
+    /// The type of the data passed over in [`JobQueue::new_job`].
+    type JobDataType: Send + Sync;
+
+    /// Instructs the driver to run this job.
+    ///
+    /// Returns true if the job was run, false if there was no capacity.
+    /// In case of an other error, the driver's callback implementation likely
+    /// wants to trigger termination of the ring and, thus, dropping of the
+    /// respective [`JobQueue`].
+    fn run_job<'a>(&self, job: JobRunnable<'a, Self::JobDataType>) -> bool;
+}
+
+#[pin_data]
+struct DriverDataWrapper<T: JobQueueOps + Send + Sync> {
+    #[pin]
+    data: T,
+}
+
+impl<T: JobQueueOps + Send + Sync> Deref for DriverDataWrapper<T> {
+    type Target = T;
+
+    fn deref(&self) -> &Self::Target {
+        &self.data
+    }
+}
+
+impl<T: JobQueueOps + Send + Sync> DriverDataWrapper<T> {
+    fn new<E>(data: impl PinInit<T, E>) -> impl PinInit<Self, E>
+        where
+            Error: From<E>,
+            E: From<Infallible>,
+    {
+        try_pin_init!(Self {
+            data <- data,
+        }? E)
+    }
+}
+
+impl<T: JobQueueOps + Send + Sync> FenceContextOps for DriverDataWrapper<T> {
+    type FenceDataType = T::JobDataType;
+}
+
+impl<'a, T: JobQueueOps + Send + Sync> JobQueue<'a, T> {
+    /// Create a new JobQueue.
+    pub fn new<E>(
+        initial_seqno: u64,
+        // TODO: pass these as &CStr. Figure out how to get the lifetimes for PinInit right.
+        driver_name: CString,
+        timeline_name: CString,
+        data: impl PinInit<T, E>,
+    ) -> impl PinInit<Self, Error>
+    where
+        Error: From<E>,
+        E: From<Infallible>,
+    {
+        try_pin_init!(&this in Self {
+            inner <- new_spinlock_irq!(try_pin_init!(JobQueueInner {
+                submit_worker_active: false,
+                // SAFETY: `this` is valid. The lifetimes ensure that the reference remains valid.
+                submit_worker <- new_scoped_work!("JobQueueSubmitWorker", Ok(SubmitWorker { jq: unsafe { &*this.as_ptr() } })),
+                waiting_jobs: List::new(),
+                running_jobs: List::new(),
+            })),
+            fctx <- FenceContext::new(initial_seqno, &driver_name, &timeline_name, DriverDataWrapper::new(data)),
+        })
+    }
+
+    /// Create a new job.
+    pub fn new_job(&'a self, data: T::JobDataType) -> Result<Job<'a, T>> {
+        let fence_allocation = self.fctx.new_fence_allocation(data)?;
+        let job = try_pin_init!(JobInternal {
+            links <- ListLinks::new(),
+            fence_allocation: ListArcField::new(Some(fence_allocation)),
+            fence: ListArcField::new(None),
+            nr_of_deps: Atomic::new(0),
+            dependencies <- ListArcField::new(KVec::new()),
+            jq: &self,
+        });
+
+        let inner = ListArc::pin_init(job, GFP_KERNEL)?;
+
+        Ok(Job { inner })
+    }
+
+    // TODO RFC the lifetime ensures that only jobs can be submitted that were
+    // actually created on this JQ, right?
+    /// Submit a job that has previously been created with [`JobQueue::new_job()`].
+    ///
+    /// Returns a [`dma_buf::Fence`] on which the user can register callbacks.
+    /// The fence will be signaled once the job is completed through
+    /// [`JobQueue::complete_jobs_up_to_seqno`], which can happen immediately
+    /// after the job has been submitted.
+    // Mutable `self` so that the user is forced to ensure synchronization.
+    // That's a bit better so that the fence sequence numbers stay ordered.
+    pub fn submit(&mut self, job: Job<'a, T>) -> ARef<Fence> {
+        let mut job = job.inner;
+        let fence = job.fence_allocation().take().expect("fence_allocation not present").new_fence();
+
+        let ret_fence = ARef::from(fence.as_fence());
+
+        *job.fence() = Some(fence);
+
+        let mut inner = self.inner.lock();
+
+        inner.as_mut().project().waiting_jobs.push_back(job);
+        inner.as_mut().check_start_submit_worker();
+
+        ret_fence
+    }
+
+    /// Complete all the jobs in the queue's running_list, up to `seqno`.
+    ///
+    /// This signals the jobs' fences and drops both job and fence.
+    pub fn complete_jobs_up_to_seqno(&self, seqno: u64, status: Result) {
+        let mut jq = self.inner.lock();
+        let mut jq = jq.as_mut().project();
+
+        let mut cursor = jq.running_jobs.cursor_front();
+
+        while let Some(job) = cursor.peek_next() {
+            let mut job = job.remove();
+
+            // TODO RFC I couldn't figure out how to access job.fence without removing first:
+            // `if job.deref().fence.get_mut().unwrap().as_fence().seqno() > seqno {` ???
+            if job.fence().as_mut().expect("fence not present").as_fence().seqno() > seqno {
+                jq.running_jobs.push_front(job);
+                return;
+            }
+
+            job.fence().take().expect("fence not present").signal(status);
+        }
+    }
+
+    // RFC:
+    // Do we want to provide a method like 
+    //
+    // pub fn complete_front_job() -> T
+    //
+    // that returns the user's data T so that it can be freed in a deferred
+    // manner should T's drop() do something illegal in atomic context?
+    //
+    // That way the JobQueue would not have to handle dropping through its own
+    // workqueue, which might be problematic synchronization-wise.
+    //
+    // I suppose it depends on how common the use-case is, i.e., what kind of
+    // data a job typically carries.
+    //
+    // On the other hand, the job's data T with this implementation lives in
+    // DriverFence's T, which already is being dropped with call_rcu(). So it
+    // might be worth at least considering whether rcu_work() might address that?
+}
+
+#[pinned_drop]
+impl<T: JobQueueOps + Send + Sync> PinnedDrop for JobQueue<'_, T> {
+    fn drop(self: Pin<&mut Self>) {
+        self.complete_jobs_up_to_seqno(u64::MAX, Err(ECANCELED));
+        // TODO: Warn if JQ drops with remaining jobs. Or maybe not? It is being
+        // proposed to drop things implicitly.
+    }
+}
diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs
index fd6ed35bc35a..f2b7fbbce5bb 100644
--- a/rust/kernel/drm/mod.rs
+++ b/rust/kernel/drm/mod.rs
@@ -8,6 +8,7 @@
 pub mod gem;
 pub mod gpuvm;
 pub mod ioctl;
+pub mod job_queue;
 
 pub use self::device::Device;
 pub use self::device::DeviceContext;
@@ -20,6 +21,9 @@
 pub use self::driver::DriverInfo;
 pub use self::driver::Registration;
 pub use self::file::File;
+pub use self::job_queue::JobQueue;
+pub use self::job_queue::JobQueueOps;
+pub use self::job_queue::JobRunnable;
 
 pub(crate) mod private {
     pub trait Sealed {}
-- 
2.55.0

Re: [RFC PATCH v4 3/3] rust: drm: Add JobQueue

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] drm: self-deadlock in JobQueue::complete_jobs_up_to_seqno when signaling fences
- [High] drm: JobQueue permanently stalls if run_job returns false
- [High] drm: JobQueue drops waiting_jobs without signaling their fences on destruction
- [High] drm: lock inversion deadlock between JobQueue lock and driver hardware locks
- [High] drm: use-after-free of ScopedWork during JobQueue destruction
- [High] drm: AB-BA lock inversion between JobQueue lock and dma_fence locks
- [Medium] drm: Job::add_dependency fails with ENOENT for already signaled fences
--

commit bfb454136546eb3a954ab9c169ab5c74abe87b91
Author: Philipp Stanner <phasta@kernel.org>

rust: drm: Add JobQueue

JobQueue is a pure Rust component that handles job submissions for GPUs
with firmware scheduling, i.e., GPUs that support one exclusive ring per
execution context.

> diff --git a/rust/kernel/drm/job_queue.rs b/rust/kernel/drm/job_queue.rs
> new file mode 100644
> index 0000000000000..9a6bc1e8fffcb
> --- /dev/null
> +++ b/rust/kernel/drm/job_queue.rs

[ ... ]

> +        // SAFETY: The `waker` is not forgotten.
> +        let registration = unsafe { FenceCallbackRegistration::new(fence, waker) };
> +        match KBox::pin_init(registration, GFP_KERNEL) {
> +            Err(e) => {
> +                let _ = job.nr_of_deps.fetch_sub(1, Relaxed);
> +                return Err(e);
> +            },

[Severity: Medium]
Does this correctly handle fences that are already signaled?

If the provided dma_fence has already signaled, 
FenceCallbackRegistration::new() will return CallbackError::AlreadySignaled.
By returning Err(e) here, this gets translated into a failure (like -ENOENT)
for the user, rather than treating the dependency as successfully satisfied.

Can we treat the already-signaled case as a success instead of aborting
the job submission?

[ ... ]

> +impl<T: JobQueueOps + Send + Sync> ScopedWorkItem for SubmitWorker<'_, T> {
> +    fn run(work: &ScopedWorkRef<Self>) {
> +        let fctx_data = work.jq.fctx.deref().deref();
> +        let mut jq = work.jq.inner.lock();
> +        let mut jq = jq.as_mut().project();
> +
> +        let mut cursor = jq.waiting_jobs.cursor_front();
> +
> +        while let Some(job) = cursor.peek_next() {
> +            if job.nr_of_deps.load(Relaxed) > 0 {
> +                break;
> +            }
> +
> +            let mut job = job.remove();
> +            let runnable_job = JobRunnable {
> +                seqno: job.fence().as_ref().expect("fence not present").as_fence().seqno(),
> +                data: job.fence().as_ref().expect("fence not present").deref(),
> +            };
> +
> +            if !T::run_job(fctx_data, runnable_job) {

[Severity: High]
Can this lead to an AB-BA lock inversion deadlock?

T::run_job() is called here with the JobQueue internal spinlock held. If a
driver implementation acquires its hardware lock inside run_job() to submit
the work, the locking order is JQ-lock -> Driver-lock.

If an IRQ handler concurrently acquires the Driver-lock and then calls
complete_jobs_up_to_seqno() (which takes the JQ-lock), the locking order
is Driver-lock -> JQ-lock, causing a deadlock.

> +                // There was no capacity left. The driver will invoke the JobQueue
> +                // later again.
> +                jq.waiting_jobs.push_front(job);
> +                break;
> +            }
> +
> +            jq.running_jobs.push_back(job);
> +        }
> +
> +        *jq.submit_worker_active = false;
> +    }
> +}
> +
> +#[pin_data]
> +struct JobQueueInner<'a, T: JobQueueOps + Send + Sync> {
> +    submit_worker_active: bool,
> +    #[pin]
> +    submit_worker: ScopedWork<SubmitWorker<'a, T>>,
> +    #[pin]
> +    waiting_jobs: List<JobInternal<'a, T>>,

[Severity: High]
Can this struct layout cause a use-after-free during destruction?

In Rust, fields are dropped in declaration order. When JobQueueInner is
dropped, submit_worker is destroyed first, followed by waiting_jobs.

When waiting_jobs are dropped, they unregister their dependencies. If an
external fence signals in this exact window, DependencyWaker::on_signal()
will fire and call check_start_submit_worker(), which will enqueue the
already-freed submit_worker into the system workqueue:

JobQueueInner::check_start_submit_worker() {
    ...
    unsafe { workqueue::system_dfl().enqueue_scoped(&*self.submit_worker) };
}

[ ... ]

> +    pub fn complete_jobs_up_to_seqno(&self, seqno: u64, status: Result) {
> +        let mut jq = self.inner.lock();
> +        let mut jq = jq.as_mut().project();
> +
> +        let mut cursor = jq.running_jobs.cursor_front();
> +
> +        while let Some(job) = cursor.peek_next() {
> +            let mut job = job.remove();
> +
> +            // TODO RFC I couldn't figure out how to access job.fence without removing first:
> +            // `if job.deref().fence.get_mut().unwrap().as_fence().seqno() > seqno {` ???
> +            if job.fence().as_mut().expect("fence not present").as_fence().seqno() > seqno {
> +                jq.running_jobs.push_front(job);
> +                return;
> +            }
> +
> +            job.fence().take().expect("fence not present").signal(status);

[Severity: High]
Does this cause a spinlock self-deadlock when dependencies exist within the
same queue?

If job B depends on job A, and job A completes, calling signal() here while
the self.inner.lock() is held will synchronously invoke job B's callback
(DependencyWaker::on_signal).

That callback then immediately tries to acquire the same JobQueue lock via
self.jq.inner.lock(), resulting in an immediate self-deadlock on the CPU.

[Severity: High]
Can this also cause an AB-BA lock inversion with dma_fence locks?

The job is removed from the list and drops at the end of this while loop
iteration, still holding the JobQueue lock. Dropping the job unregisters its
dependencies, which acquires external dma_fence locks (JQ-lock -> Fence-lock).

Concurrently, if an external dependency fence signals, dma_fence_signal()
holds the Fence-lock and invokes DependencyWaker::on_signal(), which tries
to acquire the JobQueue lock (Fence-lock -> JQ-lock).

> +        }
> +    }

[Severity: High]
If T::run_job() returns false, how does the queue ever resume?

When run_job() indicates no capacity, SubmitWorker::run exits and sets
submit_worker_active to false.

When running jobs later finish, complete_jobs_up_to_seqno() frees up
capacity but it never calls check_start_submit_worker() to wake the worker
back up. This leaves the waiting_jobs stranded and the queue permanently
stalled.

[ ... ]

> +#[pinned_drop]
> +impl<T: JobQueueOps + Send + Sync> PinnedDrop for JobQueue<'_, T> {
> +    fn drop(self: Pin<&mut Self>) {
> +        self.complete_jobs_up_to_seqno(u64::MAX, Err(ECANCELED));

[Severity: High]
Does this leave userspace processes hanging on waiting_jobs?

This cleanup routine calls complete_jobs_up_to_seqno(u64::MAX), which only
iterates over the running_jobs list. The jobs in waiting_jobs are then
destroyed via struct field drops, but their DriverFence instances will be
dropped without ever being signaled.

Any userspace process or kernel subsystem waiting on these pending jobs
would hang forever. Should waiting_jobs also be aborted and signaled here?

> +        // TODO: Warn if JQ drops with remaining jobs. Or maybe not? It is being
> +        // proposed to drop things implicitly.
> +    }
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929104703.934128-2-phasta@kernel.org?part=3