[RFC PATCH v7 0/8] rust: Add drm::JobQueue

[RFC PATCH v7 0/8] rust: Add drm::JobQueue

[Philipp Stanner]

I really hoped that I could present you guys with a v1 today, but, of
course, I discovered an issue that was overlooked over the last months.
See below.

tl;dr: If JQ1 has dependencies on JQ2, and JQ2 dies, then we also need
to inform JQ1 somehow and act accordingly (teardown).

We need to discuss the desirable solution for that problem, thus I'm
requesting comments by you guys and remain on RFC for now.

Based on drm-rust-next plus ScopedWork by Danilo & Alice [1] and my
lock-error-fix [2]. My branch with a minimalist tester (my old tester updated
by Gary) can be found here [3].

[1] https://lore.kernel.org/rust-for-linux/20261020-rust-workqueue-v3-0-ea7c74d02068@google.com/
[2] https://lore.kernel.org/rust-for-linux/20261022160725.2215244-2-phasta@kernel.org/
[3] https://gitlab.freedesktop.org/pstanner/linux-drm-work/-/tree/jobqueue-jobqueue-rfc-v7-2?ref_type=heads

Changes in v7:
  - Unfortunately, I discovered a design question that we completely
    overlooked so far: If a "neighboring" JQ dies, its fences get
    signaled with an error – but the callbacks registered on a fence
    don't query that error, leading to UB by that GPU ring relying on
    the work being done. Thus:
      - Always inform about errors in
	FenceCallbackRegistration::on_signal() about errors.
      - Similarly, replace Fence::is_signaled(). It's a useless function.
	You *must* query whether a fence got signaled *successfully*.
      - !!! RFC: Add a JobQueueOps::kill() to tell the driver to tear this
	JQ down.
  - Add ID accessors for FenceCtx and JQ.
  - By Gary: Working self references + PinInit. Kudos to Gary!
      - Among the many changes that he did is a portation to a new
	minimalist list-type, since the default list collides with
	self-references.

Changes in v6:
  - Rework DmaFence so you can signal fences without dropping them
    (since we'd have to drop in atomic context otherwise)
  - Use synchronize_rcu() for fence dropping for now – with a fast path
    optimization! – instead of call_rcu() because the latter can run in
    atomic context.
  - Add DropWorker for JobQueue which drops jobs and their fences in
    process context.
  - Rework locking design: All JQ API functions can be invoked without
    any driver locks. run_job() is not executed with the JQ lock held,
    thus the driver can sleep.
  - Add a seqno based approach to catch potential races with the
    unlocked call to run_job().
  - Redesign JobQueue::complete_jobs_up_to_seqno() so that by default
    jobs are completed with status == Ok(()). This is done because it
    makes the race-catcher above easier to implement (otherwise we'd
    have to carry the last error around, and if we ever implement batch
    submissions we'd need a list of errors).
  - Since our design assumption is that users will always close their
    ring and drop their JQ if anything goes wrong on the ring, just
    provide one function that signals with an error on teardown:
    JobQueue::teardown().
  - Move the work items out of the locked JobQueueInner, since the work
    queue lock already synchronizes everything. The
    Revocable<JobQueueInner> needs to stay, however, so that the
    SubmitWorker can be safely cancelled on JQ::drop().
  - Fix various logical bugs that I found (code is unfortunately still
    untested).
  - Extend documentation significantly.
  - Formatting.

Changes in v5:
  - Add back the Revocable<JobQueueInner…> from my earliest RFCs. Reason
    is still the same: JQ dropping while dependency fences signal could
    deadlock. Similarly, this solves a dependency kicking of the work
    item again while drop() is running.
  - Do not call run_job() with a lock held, since run_job() might sleep.
    AFAICS, this also makes the proposed XArray solution impossible and
    demands that we use a list.
  - Re-add the earlier credit-count-system, so that run_job() is
    infallible. Simplifies the design quite a bit. (Danilo)
  - Drop jobs in a deferred manner through the work item so that job
    payload data can do atomic-hostile stuff.
  - Because of the point above, I suggest that we stop dropping a
    DriverFence on signal (Danilo's and my idea). It has no real
    advantage, and getting rid of it allows for better controlling what
    drops when through JobQueue. For fence it's only relevant that there
    is an RCU grace period, thus:
  - Replace DriverFence::drop()'s call_rcu() with synchronize_rcu(). We
    would, therefore, demand that everyone who's got a problem with the
    delay drops via work item.

This can still not be tested as I'm blocked by pin-init vs self-ref.
Just FYI.

I would appreciate confirmation of these following thoughts on locking
design:

1.
I believe that callers of jq.new_job() and jq.submit_job() do not need
an outer serialization mutex. Reason is that JQ serializes with its
internal lock. Sequence numbers cannot take over older seqnos because it
is submit_job() who sets the seqno.

2.
I, furthermore, believe that a driver will not need to hold a
driver-lock while calling jq.complete_jobs_up_to_seqno().

IOW, I am suggesting that a driver can always safely use JQ without an
outer lock, and actually *should* do so to avoid potential locking
issues.

Reason:

let driver_stuff = foo.lock();
let current_gpu_seqno = driver_stuff.get_seqno();
drop(driver_stuff);
jq.complete_jobs_up_to_seqno(current_gpu_seqno);

Thus, I believe that it is OK for me to take the JQ lock in
jq.complete_jobs_up_to_seqno().

Please correct me if I'm missing something.

Should I be wrong and a lock inversion could occur, then we could
hypothetically add a fence Signaler object, with which we could signal
fences with a different lock. Similar to how drm_sched does it with the
hardware_fence.


Regards
P.


====

Cover letter from v4:

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/

Gary Guo (1):
  samples: rust: Add JobQueue Tester

Philipp Stanner (7):
  rust: DmaFence: Implement Deref for FenceContext
  rust: DmaFence: Add ID accessor for fctx
  rust: DmaFence: Don't drop on signal
  rust: DmaFence: Replace call_rcu() with synchronize_rcu()
  rust: DmaFence: Replace is_signaled() with status()
  rust: DmaFence: Tell results in signaling callbacks
  rust: drm: Add JobQueue

 rust/helpers/dma_fence.c             |   4 +-
 rust/kernel/dma_buf/dma_fence.rs     | 225 ++++----
 rust/kernel/dma_buf/mod.rs           |   3 +-
 rust/kernel/drm/job_queue.rs         | 826 +++++++++++++++++++++++++++
 rust/kernel/drm/mod.rs               |   4 +
 samples/rust/Kconfig                 |  11 +
 samples/rust/Makefile                |   1 +
 samples/rust/rust_jobqueue_tester.rs | 242 ++++++++
 8 files changed, 1195 insertions(+), 121 deletions(-)
 create mode 100644 rust/kernel/drm/job_queue.rs
 create mode 100644 samples/rust/rust_jobqueue_tester.rs

-- 
2.56.0

[RFC PATCH v7 1/8] 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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
index a1d851656562..5300448882e7 100644
--- a/rust/kernel/dma_buf/dma_fence.rs
+++ b/rust/kernel/dma_buf/dma_fence.rs
@@ -86,6 +86,15 @@ pub struct FenceContext<T: FenceContextOps + Send + Sync> {
     data: T,
 }
 
+impl<T: Send + Sync + FenceContextOps> Deref for FenceContext<T> {
+    type Target = T;
+
+    #[inline]
+    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.56.0

[RFC PATCH v7 2/8] rust: DmaFence: Add ID accessor for fctx

[Philipp Stanner]

A FenceContext has a fence context number, which can serve as an ID
field. This number can be useful for owners of multiple contexts to
identify them, store them in an XArray etc.

Add an accessor for fctx.nr.

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

diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
index 5300448882e7..897308f60dd3 100644
--- a/rust/kernel/dma_buf/dma_fence.rs
+++ b/rust/kernel/dma_buf/dma_fence.rs
@@ -135,6 +135,12 @@ pub fn new<E>(
         })
     }
 
+    /// Obtain the FenceContext's ID number.
+    #[inline]
+    pub fn id(&self) -> u64 {
+        self.nr
+    }
+
     fn next_seqno(&self) -> u64 {
         self.seqno.fetch_add(1, Relaxed)
     }
-- 
2.56.0

[RFC PATCH v7 3/8] rust: DmaFence: Don't drop on signal

[Philipp Stanner]

The original design for DmaFence intended to comply as strictly as
possible with the "dma_fence contract", which, according to some, says
that a fence must only be signaled once. Thus, the implementation was
made so that a DriverFence drops on signal.

While extending JobQueue's capabilities, it was discovered that this
characteristic causes - unnecessary - trouble: fence-data must be
dropped in an RCU-deferred manner because the C backend demands that.
However, fences are often signaled in atomic context (e.g., interrupt
handler) where you don't want to drop, but signal as quickly as
possible.

Actually, there is no real hard reason to enforce that a fence can only
get signaled once, because the C backend is robust against multiple
signal attempts.

What is decisive is that a) all fences signal and b) they only drop no
earlier than 1 RCU grace period after signaling.

Don't drop a DriverFence on signal().

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

diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
index 897308f60dd3..c04b68fe67fe 100644
--- a/rust/kernel/dma_buf/dma_fence.rs
+++ b/rust/kernel/dma_buf/dma_fence.rs
@@ -820,7 +820,7 @@ pub fn as_fence(&self) -> &Fence {
     }
 
     /// Signal the fence. This will invoke all registered callbacks.
-    pub fn signal(self, res: Result) {
+    pub fn signal(&self, res: Result) {
         let fence = self.as_fence().lock();
 
         // SAFETY: `fence` is valid because `self` is valid. The lock must be
@@ -831,14 +831,15 @@ pub fn signal(self, res: Result) {
                 // must not have been signaled yet, which we check directly above.
                 unsafe { bindings::dma_fence_set_error(fence.as_raw(), err.to_errno()) };
             }
+
+            // SAFETY: `self.data` is valid because `self` is valid.
+            let fctx = unsafe { self.data.as_ref().fctx };
+            let _ = fctx.nr_of_unsignaled_fences.fetch_sub(1, Relaxed);
+
             // SAFETY: `fence` is valid because `self` is valid. The lock must
             // be held, which we acquired above.
             unsafe { bindings::dma_fence_signal_locked(fence.as_raw()) };
         }
-
-        // SAFETY: `self.data` is valid because `self` is valid.
-        let fctx = unsafe { self.data.as_ref().fctx };
-        let _ = fctx.nr_of_unsignaled_fences.fetch_sub(1, Relaxed);
     }
 }
 
-- 
2.56.0

[RFC PATCH v7 4/8] rust: DmaFence: Replace call_rcu() with synchronize_rcu()

[Philipp Stanner]

The C dma_fence contract demands that a fence's data disappears no
earlier than 1 RCU grace period after the DriverFence was signaled.

So far, we achieved this with dropping the DriverFence's data in a
deferred manner with a manual, raw binding call to call_rcu().

However, this call_rcu() does not solve one problem: It is conceivable
that some driver data must not drop in atomic context; but call_rcu()'s
payload *can* be executed in atomic context.

Moreover, the current developments in drm::JobQueue, where a DriverFence
is always coupled 1:1 with a Job, allows for dropping job and fence
through a work item.

The undesirable blocking behavior of synchronize_rcu() would, thus, not
be annoying any thread anymore. The direct users of DmaFence could
achieve the same by dropping their DriverFences through work items.

The only conceivable other solution, queue_rcu_work(), would, for many
users, schedule a work_item from another work_item, which seems
undesirable.

Additionally, using the existing synchronize_rcu() abstraction has the
advantage of us getting rid of a raw bindings call.

Replace DriverFence::drop()'s call_rcu() with synchronize_rcu().

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

diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
index c04b68fe67fe..d26cf9e8e4b1 100644
--- a/rust/kernel/dma_buf/dma_fence.rs
+++ b/rust/kernel/dma_buf/dma_fence.rs
@@ -42,7 +42,7 @@
             Atomic,
             Relaxed, //
         },
-        rcu::rcu_barrier, //
+        rcu::synchronize_rcu, //
     }, //
 };
 
@@ -65,7 +65,7 @@ pub struct FenceContext<T: FenceContextOps + Send + Sync> {
     /// The sequence number for the next fence created.
     seqno: Atomic<u64>,
     // The name parameters can be accessed by the dma_fence backend_ops. UAF
-    // errors are prevented by the `call_rcu()` in `drop_driver_fence_data()`.
+    // errors are prevented by `synchronize_rcu()`.
     /// The name of the driver this FenceContext's fences belong to.
     driver_name: CString,
     /// The name of the timeline this FenceContext's fences belong to.
@@ -149,15 +149,11 @@ fn next_seqno(&self) -> u64 {
     ///
     /// This is needed because many times, creation of a [`DriverFence`] must not
     /// fail, and allocating might deadlock in some situations.
-    ///
-    /// The `data` you pass here must not perform any operations that are illegal
-    /// in atomic context in its [`Drop`] implementation.
     pub fn new_fence_allocation(
         &self,
         data: T::FenceDataType,
     ) -> Result<DriverFenceAllocation<'_, T>> {
         let fence_data = DriverFenceData {
-            rcu_head: Default::default(),
             // `inner` remains uninitialized until a `DriverFence` takes over.
             inner: Fence {
                 inner: Opaque::uninit(),
@@ -243,15 +239,10 @@ fn drop(self: Pin<&mut Self>) {
             panic!("Forgotten fences in FenceContext.");
         }
 
-        // Ensure that the driver cannot unload while there are still dma_fence
-        // callbacks running. At the same time, the RCU barrier addresses the
-        // problem inherited by the C backend, in which backend ops callbacks
-        // might be accessing the fence while it is being signaled (or shortly
-        // after). This could cause UAF access on the fence context's
-        // `fctx.driver_name` and `fctx.timeline_name`.
-        //
-        // Wait for the RCU callbacks in `DriverFence::drop`.
-        rcu_barrier();
+        // If a DriverFence signals without doing a synchronize_rcu(), and a
+        // fctx drops immediately afterwards, then backend_ops still accessing
+        // the fctx might run into UAF if we don't wait for them here.
+        synchronize_rcu();
     }
 }
 
@@ -615,12 +606,9 @@ struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> {
     // necessary so that the C backend can free the allocation (coming from our
     // Rust code) with kfree_rcu().
     inner: Fence,
-    /// Callback head for dropping this in a deferred manner through RCU.
-    rcu_head: bindings::callback_head,
     /// Reference to access the FenceContext.
     fctx: &'a FenceContext<T>,
-    /// The API user's data. It is essential that the data only performs
-    /// operations legal in atomic context in its [`Drop`] implementation.
+    /// The API user's data.
     #[pin]
     data: T::FenceDataType,
 }
@@ -955,15 +943,15 @@ fn drop(&mut self) {
         // dma_fence_is_signaled_locked() because the C backend wants to get rid
         // of the latter.
 
-        // SAFETY: `guard` is valid until the `call_rcu()` below.
+        // SAFETY: `guard` is valid.
         let signaled: bool = unsafe { bindings::dma_fence_test_signaled_flag(guard.as_raw()) };
         if !signaled {
             pr_err!("DriverFence drops unsignaled. Danger of memory corruption!\n");
-            // SAFETY: `guard` is valid until the `call_rcu()` below. The fence
-            // must not have been signaled yet, which we check directly above.
+            // SAFETY: `guard` is valid. The fence must not have been signaled
+            // yet, which we check directly above.
             unsafe { bindings::dma_fence_set_error(guard.as_raw(), ECANCELED.to_errno()) };
-            // SAFETY: `guard` is valid until the `call_rcu()` below. The lock
-            // must be held, which we acquired above.
+            // SAFETY: `guard` is valid. The lock must be held, which we
+            // acquired above.
             unsafe { bindings::dma_fence_signal_locked(guard.as_raw()) };
 
             // SAFETY: `self.data` is valid because `self` is valid.
@@ -988,59 +976,18 @@ fn drop(&mut self) {
             return;
         }
 
-        // SAFETY: Valid because `self` is valid.
-        let rcu_head_ptr = unsafe { &raw mut (*self.data.as_ptr()).rcu_head };
+        // Make sure none of the fence backend_ops can access fence-data anymore.
+        //
+        // TODO:
+        // This would not be necessary if the C dma_fence backend were using a
+        // spinlock to properly synchronize its signaled state. Fix it in C and
+        // then remove synchronize_rcu().
+        synchronize_rcu();
 
-        // SAFETY: `call_rcu()` is always safe to be called. `rcu_head_ptr` was
-        // created validly above. The module must perform a `synchronize_rcu()`
-        // or `rcu_barrier()` call to guard against module unload.
-        unsafe { bindings::call_rcu(rcu_head_ptr, Some(drop_driver_fence_data::<T>)) };
+        // SAFETY: Valid because `self` is valid.
+        unsafe { drop_in_place(&raw mut self.data) };
+
+        // SAFETY: The `synchronize_rcu()` above ensures all accessors are gone.
+        unsafe { bindings::dma_fence_put(self.as_raw()) };
     }
 }
-
-// TODO:
-// The entire call_rcu() mechanism in the drop above and the code below would be
-// unnecessary if C's dma_fence_signal() could be reworked in a way that after it
-// ran, the caller knows that no fence_ops callbacks can be running anymore.
-// In other words, if the dma_fence backend would use its spinlock for full
-// synchronization.
-//
-// Then we could move the drop_in_place() and dma_fence_put() upwards into the
-// drop() implementation and call it a day.
-
-/// Finally really drop this `DriverFence<T>`
-///
-/// # Safety
-///
-/// `head` references the `rcu_head` field of an `DriverFenceData<T>`. All
-/// accessors to that `DriverFenceData<T>` must be gone by now. This must be
-/// ensured by signalling the associated `DriverFence<T>` and then waiting
-/// for a grace period until calling this function here.
-unsafe extern "C" fn drop_driver_fence_data<T: Send + Sync + FenceContextOps>(
-    head: *mut bindings::callback_head,
-) {
-    // SAFETY: Caller provides a pointer to the `rcu_head` field of a `DriverFenceData<C>`.
-    let fence_data = unsafe { container_of!(head, DriverFenceData<'_, T>, rcu_head) };
-
-    // SAFETY: `fence_data` was created validly above. All the fence's data will
-    // only drop below, but the raw pointer to the raw C `dma_fence` remains
-    // valid because the reference count is only decremented at the end of the
-    // function.
-    let fence = unsafe { (*fence_data).inner.inner.get() };
-
-    // SAFETY: `fence_data` was created validly above. The user has already
-    // dropped the only conventional accessor to the user data, the `DriverFence`,
-    // one grace period ago. All accessors are gone now.
-    unsafe { drop_in_place(&raw mut (*fence_data).data) };
-
-    // The inner `Fence` explicitly does not get dropped because there may be
-    // many more users / consumers, each holding their own reference.
-
-    // SAFETY: Once a `DriverFence` is initialized, the inner `fence` is valid
-    // and initialized. It is valid until the refcount drops to 0, which can
-    // earliest happen once we drop the `DriverFence`'s reference here.
-    unsafe { bindings::dma_fence_put(fence) };
-
-    // The actual memory the data associated with a `DriverFence` lives in
-    // gets freed by the C dma_fence backend once the fence's refcount reaches 0.
-}
-- 
2.56.0

[RFC PATCH v7 5/8] rust: DmaFence: Replace is_signaled() with status()

[Philipp Stanner]

As it turns out, our dma_fence abstractions lack an essential feature
that is partially derived from the C backend:

Despite it being a common pattern in the kernel, you actually cannot
just check whether a fence was signaled and call it a day - it is
crucial that you also check whether a fence was signaled *with an
error*.

Making things more complicated, an error can get modified at any point
in time before a fence gets signaled. Thus, the only solid way of
obtaining the error code is by simultaneously checking the
signaled-status under lock-protection.

Since the fence status is essential information, replace
Fence::is_signaled() with Fence::status(), making it impossible to
ignore the error code.

Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 rust/helpers/dma_fence.c         |  4 +-
 rust/kernel/dma_buf/dma_fence.rs | 70 +++++++++++++++++++-------------
 rust/kernel/dma_buf/mod.rs       |  3 +-
 3 files changed, 45 insertions(+), 32 deletions(-)

diff --git a/rust/helpers/dma_fence.c b/rust/helpers/dma_fence.c
index 549f6b6a7171..9d8eb7db19bf 100644
--- a/rust/helpers/dma_fence.c
+++ b/rust/helpers/dma_fence.c
@@ -22,9 +22,9 @@ __rust_helper void rust_helper_dma_fence_end_signalling(bool cookie)
 	dma_fence_end_signalling(cookie);
 }
 
-__rust_helper bool rust_helper_dma_fence_is_signaled(struct dma_fence *f)
+__rust_helper bool rust_helper_dma_fence_is_signaled_locked(struct dma_fence *f)
 {
-	return dma_fence_is_signaled(f);
+	return dma_fence_is_signaled_locked(f);
 }
 
 __rust_helper bool rust_helper_dma_fence_test_signaled_flag(struct dma_fence *f)
diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
index d26cf9e8e4b1..71965b26e148 100644
--- a/rust/kernel/dma_buf/dma_fence.rs
+++ b/rust/kernel/dma_buf/dma_fence.rs
@@ -502,40 +502,48 @@ unsafe impl Send for Fence {}
 // SAFETY: Fences are literally designed to be shared between threads.
 unsafe impl Sync for Fence {}
 
+/// Fence-Status queried through [`Fence::status()`].
+#[derive(Debug)]
+pub enum FenceStatus {
+    /// The fence is not yet signaled.
+    Unsignaled,
+    /// The fence is signaled. Might have been signaled with an error.
+    Signaled(Result),
+}
+
 impl Fence {
+    /// Require a fence's status while holding the lock.
+    ///
+    /// # Safety
+    ///
+    /// The dma_fence lock must be held when calling this function.
+    unsafe fn status_locked(&self) -> FenceStatus {
+        // SAFETY: Inner `fence` is valid because `self` is valid.
+        let signaled = unsafe { bindings::dma_fence_is_signaled_locked(self.as_raw()) };
+
+        if signaled {
+            // SAFETY: `self` is valid and we acquired the lock above.
+            let err = unsafe { (*self.as_raw()).error };
+
+            return FenceStatus::Signaled(to_result(err));
+        }
+
+        return FenceStatus::Unsignaled;
+    }
+
     /// Check whether the fence was signaled at the moment of the function call.
+    /// If it is signaled, return the fence's status, i.e., determine if it was
+    /// signaled with an error.
     ///
     /// Note that this can return `true` for a [`Fence`] whose [`DriverFence`]
     /// has not yet been dropped. The reason is that the fence ops callbacks can
     /// cause the fence to get signaled by the C backend.
     #[inline]
-    pub fn is_signaled(&self) -> bool {
-        // We should not use `dma_fence_is_signaled_locked()` here, because
-        // according to the C backend's recommendations, that function is
-        // problematic and we should avoid calling that function with a lock
-        // held.
+    pub fn status(&self) -> FenceStatus {
+        let fence = self.lock();
 
-        // SAFETY: Inner `fence` is valid because `self` is valid.
-        let ret = unsafe { bindings::dma_fence_is_signaled(self.as_raw()) };
-
-        // To be as robust as possible for the future we guarantee that an API
-        // caller can 100% rely on the signaling being completed (i.e., all
-        // fence callbacks ran), so we have to take the lock.
-        //
-        // The reason is that the C dma_fence backend currently does not
-        // carefully synchronize the `dma_fence_is_signaled()` function with the
-        // proper spinlock. This can lead to the function returning `true` while
-        // fence callbacks are still being executed. This can be mitigated by
-        // guarding the entire function with the spinlock.
-        //
-        // The fundamental reason is that the C backend currently does guard
-        // setting of the fence's signaled-bit with the fence's spinlock, but
-        // reading is done locklessly.
-        //
-        // See commit c8a5d5ea3ba6a.
-        let _ = self.lock();
-
-        ret
+        // SAFETY: We required the lock directly above.
+        unsafe { fence.status_locked() }
     }
 
     /// Lock the fence. A helper only to be used internally in this module.
@@ -657,11 +665,15 @@ struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> {
 ///         FenceContextOps,
 ///         FenceCallback,
 ///         FenceCallbackRegistration,
+///         FenceStatus, //
 ///     },
 ///     str::CString,
 ///     sync::aref::ARef, //
 /// };
-/// use core::fmt::Display;
+/// use core::{
+///     assert_matches,
+///     fmt::Display, //
+/// };
 ///
 /// struct CallbackData { }
 ///
@@ -708,9 +720,9 @@ struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> {
 /// let cb_reg = KBox::pin_init(cb_reg, GFP_KERNEL)?;
 ///
 /// // TODO signalling guards
-/// assert_eq!(waiting_fence.is_signaled(), false);
+/// assert_matches!(waiting_fence.status(), FenceStatus::Unsignaled);
 /// fence.signal(Ok(()));
-/// assert_eq!(waiting_fence.is_signaled(), true);
+/// assert_matches!(waiting_fence.status(), FenceStatus::Signaled(Ok(())));
 ///
 /// Ok::<(), Error>(())
 /// ```
diff --git a/rust/kernel/dma_buf/mod.rs b/rust/kernel/dma_buf/mod.rs
index 4764a828642e..b5254144f69e 100644
--- a/rust/kernel/dma_buf/mod.rs
+++ b/rust/kernel/dma_buf/mod.rs
@@ -10,5 +10,6 @@
     FenceCallback,
     FenceCallbackRegistration,
     FenceContext,
-    FenceContextOps, //
+    FenceContextOps,
+    FenceStatus, //
 };
-- 
2.56.0

[RFC PATCH v7 6/8] rust: DmaFence: Tell results in signaling callbacks

[Philipp Stanner]

A FenceCallbackRegistration invokes the on_signal() trait functions
specified by the user.

These callbacks always get executed, even if the fence was signaled with
an error. The user must then be able to react accordingly.

Since the fence->error must only be set before a fence signals, and must
not change after it signaled, we better acquire it with the lock.

Change the on_signal() trait function so that the user always gets
passed the fence's actual state.

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

diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
index 71965b26e148..c8856dbed0a2 100644
--- a/rust/kernel/dma_buf/dma_fence.rs
+++ b/rust/kernel/dma_buf/dma_fence.rs
@@ -291,9 +291,12 @@ fn from(e: AllocError) -> Self {
 /// }
 ///
 /// impl FenceCallback for MyCallback {
-///     fn on_signal(&mut self) {
-///         pr_info!("Fence signaled!\n");
-///         // Handle fence completion
+///     fn on_signal(&mut self, status: Result) {
+///         if status.is_ok() {
+///             pr_info!("Fence signaled!\n");
+///         } else {
+///             pr_err!("Fence signaled with an error!\n");
+///         }
 ///     }
 /// }
 /// ```
@@ -308,7 +311,7 @@ pub trait FenceCallback: Send {
     /// An implementation likely wants to inform itself (e.g., through a work item)
     /// within this callback that the associated [`FenceCallbackRegistration`]
     /// can now be dropped.
-    fn on_signal(&mut self);
+    fn on_signal(&mut self, status: Result);
 }
 
 /// A callback registration on a fence.
@@ -395,11 +398,18 @@ pub unsafe fn new<'a>(
         // embedded into a `FenceCallbackRegistration`.
         let reg: *mut Self = unsafe { container_of!(ptr, Self, callback_foreign) };
 
+        // SAFETY: The lock is being held by the C backend.
+        let fence_result = match unsafe { (*reg).fence.status_locked() } {
+            FenceStatus::Signaled(result) => result,
+            // Can never happen.
+            FenceStatus::Unsignaled => Err(ENOENT),
+        };
+
         // SAFETY: `reg` is a valid `Self` pointer.
         //
         // The backend ensures synchronisation so whoever holds the registration object cannot drop
         // it while this code is running. See `FenceCallbackRegistration::drop`.
-        unsafe { (*reg).callback.on_signal() };
+        unsafe { (*reg).callback.on_signal(fence_result) };
     }
 
     /// Returns a reference to the fence this callback is registered on.
@@ -678,8 +688,12 @@ struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> {
 /// struct CallbackData { }
 ///
 /// impl FenceCallback for CallbackData {
-///     fn on_signal(&mut self) {
-///         pr_info!("DmaFence callback executed.\n");
+///     fn on_signal(&mut self, status: Result) {
+///         if status.is_ok() {
+///             pr_info!("DmaFence callback executed.\n");
+///         else {
+///             pr_err!("DmaFence callback executed, but fence has an error.\n");
+///         }
 ///     }
 /// }
 ///
-- 
2.56.0

[RFC PATCH v7 7/8] 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 fulfilled 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.

The submit_worker runs all ready jobs based on a credit count system, so
that run_job() can be infallible. The same worker drops jobs (and their
fences) that are already completed. Should this proof to be a
performance issue, a second worker could trivially be added for this.

Signed-off-by: Philipp Stanner <phasta@kernel.org>
Co-developed-by: Gary Guo <gary@garyguo.net>

(^ waiting for Gary to sign-off)
---
 rust/kernel/drm/job_queue.rs | 826 +++++++++++++++++++++++++++++++++++
 rust/kernel/drm/mod.rs       |   4 +
 2 files changed, 830 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..e51adedaeddb
--- /dev/null
+++ b/rust/kernel/drm/job_queue.rs
@@ -0,0 +1,826 @@
+// 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 can spawn one ring exclusive per user context.
+//!
+//! JobQueue abstracts the entire handling of [`dma_buf::FenceContext`] and its
+//! associated [`dma_buf::DriverFence`]s for the user.
+//!
+//! In this design, a JobQueue is always the owner of a [`dma_buf::FenceContext`]
+//! which represents the timeline on the associated GPU ring. Jobs created on
+//! this JobQueue always contain a [`dma_buf::DriverFence`] from this JobQueue,
+//! where the fence sequence number corresponds with the job sequence number,
+//! and the fence's generic payload data is also the job's payload data. This
+//! allows for the dma_fence backend_ops callbacks interacting with the driver
+//! through JobQueue. The trait [`JobQueueOps`] is the interface between driver
+//! and fence backend_ops.
+//!
+//! [`Job`]s created on the [`JobQueue`] take over ownership of the user's data
+//! and prepare all memory allocations. A later job submission sets up the fence
+//! and performs no fallible operations.
+//!
+//! Before jobs are submitted to a queue, dependencies in the form of
+//! [`dma_buf::Fence`] can be set on them. Jobs are run by the JobQueue in
+//! process context through a work item in FIFO order. Only jobs whose
+//! dependency fences have all been signaled are run. If the front job's
+//! dependency is not yet fulfilled, the JobQueue pauses until that's the case.
+//!
+//! JobQueue implements a credit count system, where the queue itself gets
+//! assigned a capacity by the user, and jobs get assigned individual cost. Jobs
+//! get only pushed if there is enough capacity; if not, running stops until
+//! that is the case. These credits must accurately represent the capacity on
+//! the GPU ring the queue was created for.
+//!
+//! The driver tells the JobQueue which jobs are finished through
+//! [`JobQueue::complete_jobs_up_to_seqno()`]. This function is typically
+//! invoked through the driver's GPU interrupt handler.
+//!
+//! Since jobs might contain data that must not drop in atomic context, and
+//! since jobs' [`dma_buf::DriverFence`]s perform a call to
+//! [`rcu::synchronize_rcu()`], they are dropped through a work item if [`Drop`]
+//! is implemented for the job's payload data.
+//!
+//! From JobQueue's perspective, it is not necessary to call any of its API
+//! functions with a driver lock held; this is actually discouraged to reduce
+//! the probability of deadlocks. All JobQueue API functions are properly
+//! ordered using the internal lock.
+//!
+//! JobQueue tells drivers to run jobs through [`JobQueueOps::run_job`], which
+//! is called without the JobQueue's spinlock being held, i.e., the driver is
+//! permitted to perform sleeping operations in the callback. JobQueue handles
+//! races between `run_job` and [`JobQueueOps::complete_jobs_up_to_seqno()`].
+//!
+//! This is pure Rust code that does not directly abstract C.
+
+#![allow(clippy::multiple_bound_locations)] // Clippy false positive, to be addressed by pin-init.
+
+use core::{
+    convert::Infallible,
+    ops::Deref, //
+};
+
+use kernel::{
+    dma_buf::dma_fence::{
+        DriverFence,
+        DriverFenceAllocation,
+        Fence,
+        FenceCallback,
+        FenceCallbackRegistration,
+        FenceContext,
+        FenceContextOps, //
+    },
+    list::{
+        impl_list_arc_safe,
+        impl_list_item,
+        List,
+        ListItem,
+        ListLinks, //
+    },
+    new_srcu,
+    prelude::*,
+    str::CString,
+    sync::{
+        aref::ARef,
+        atomic::{
+            Atomic,
+            Relaxed, //
+        }, //
+        new_spinlock_irq,
+        SpinLockIrq,
+    }, //
+    sync::{
+        atomic::AtomicFlag,
+        Srcu, //
+    },
+    sync::{Arc, UniqueArc},
+    workqueue::{
+        self,
+        new_scoped_work,
+        //        ScopedQueue,
+        ScopedWork,
+        ScopedWorkItem,
+        ScopedWorkRef,
+    }, //
+};
+
+/// A linked list where all elements are owned.
+///
+/// This is a thin wrapper of `List` with all elements are `UniqueArc`.
+struct UniqueList<T: ListItem>(List<T>);
+
+impl<T: ListItem> Default for UniqueList<T> {
+    fn default() -> Self {
+        Self::new()
+    }
+}
+
+impl<T: ListItem> UniqueList<T> {
+    fn new() -> Self {
+        Self(List::new())
+    }
+
+    fn push_front(&mut self, item: Pin<UniqueArc<T>>) {
+        self.0.push_front(item.into())
+    }
+
+    fn push_back(&mut self, item: Pin<UniqueArc<T>>) {
+        self.0.push_back(item.into())
+    }
+
+    fn pop_front(&mut self) -> Option<Pin<UniqueArc<T>>> {
+        Some(
+            Arc::into_unique_or_drop(self.0.pop_front()?.into_arc())
+                .expect("UniqueList should only contain UniqueArc"),
+        )
+    }
+}
+
+struct DependencyWaker<'b, 'nr_of_deps, T: JobQueueOps + Send + Sync> {
+    nr_of_deps: &'nr_of_deps Atomic<u32>,
+    jq: &'b JobQueue<T>,
+}
+
+impl<'b, T: JobQueueOps + Send + Sync> FenceCallback for DependencyWaker<'b, '_, T> {
+    fn on_signal(&mut self, res: Result) {
+        if let Err(err) = res {
+            // Dependencies are unfulfilled and will never be fulfilled. Halt the
+            // submit_worker and tell the driver to tear this JQ down.
+            self.jq.revoked.store(true, Relaxed);
+            self.jq.srcu.synchronize();
+
+            // TODO RFC:
+            // We likely want a work_item for this, don't we? Because the
+            // dma_fence backend is holding the lock.
+            T::kill(self.jq.fctx.deref(), self.jq.id(), err);
+        }
+
+        // Last dependency kicks the worker.
+        if self.nr_of_deps.fetch_sub(1, Relaxed) == 1 {
+            // The drop-order of JobQueue ensures that all DependencyWakers are
+            // cancelled before the JobQueue itself is dropped.
+            self.jq.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: Pin<UniqueArc<JobInternal<'a, T>>>,
+    fence_allocation: DriverFenceAllocation<'a, DriverDataWrapper<T>>,
+}
+
+impl<'a, T: JobQueueOps + Send + Sync> Job<'a, T> {
+    /// Add a dependency for the job.
+    ///
+    /// A JobQueue will only push a job once `fence` has been signaled.
+    pub fn add_dependency(&mut self, fence: &Fence) -> Result {
+        self.inner.as_mut().with_project(|job| {
+            // 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);
+
+            let dependency = try_pin_init!(Dependency {
+                links <- ListLinks::new(),
+                // SAFETY: The `registration` is not forgotten. It drops with JobQueue.
+                registration <- unsafe {
+                    FenceCallbackRegistration::new(
+                        fence,
+                        DependencyWaker {
+                            nr_of_deps: job.nr_of_deps,
+                            jq: job.jq,
+                        },
+                    )
+                }
+            });
+            match UniqueArc::pin_init(dependency, GFP_KERNEL) {
+                Err(e) => {
+                    let _ = job.nr_of_deps.fetch_sub(1, Relaxed);
+                    if e == ENOENT {
+                        // Was already signaled. No reason to bother the caller.
+                        return Ok(());
+                    }
+                    return Err(e);
+                }
+                Ok(dep) => {
+                    job.dependencies.push_back(dep);
+                }
+            }
+
+            Ok(())
+        })
+    }
+
+    /// Submit the 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.
+    ///
+    /// This function does all fence initialization under the JobQueue lock in
+    /// one go, including setting of the fence sequence number. Thus, it is
+    /// typically unnecessary (and advaised against) to call this function with
+    /// a driver lock held.
+    pub fn submit(self) -> ARef<Fence> {
+        let jq = self.inner.jq;
+        jq.with_inner(|inner| {
+            // SAFETY: this is a lifetime transmute from `Job<'queue, T>` to
+            // `Job<'fctx, T>`.
+            //
+            // `job` internally holds a reference to `JobQueue`, while we are
+            // now moving `job` into `inner`, which may borrow from `fctx` but
+            // not the full `JobQueue`. So on surface this would be incorrect.
+            // However, as we drain `inner` inside `JobQueue::drop`, the
+            // reference is gone before any field is destructed.
+            let mut job = unsafe { core::mem::transmute::<Job<'_, T>, Job<'_, T>>(self) };
+
+            // The lock orders setting of the sequence number, so no driver lock is needed.
+            let mut inner = inner.lock();
+            let fence = job.fence_allocation.new_fence();
+            let ret_fence = ARef::from(fence.as_fence());
+
+            *job.inner.as_mut().project().fence = Some(fence);
+
+            inner.waiting_jobs.push_back(job.inner);
+            drop(inner);
+            jq.check_start_submit_worker();
+
+            ret_fence
+        })
+    }
+}
+
+#[pin_data]
+struct Dependency<'b, 'nr_of_deps, T: JobQueueOps + Send + Sync> {
+    #[pin]
+    links: ListLinks,
+    #[pin]
+    registration: FenceCallbackRegistration<DependencyWaker<'b, 'nr_of_deps, T>>,
+}
+
+impl_list_arc_safe! {
+    impl{'b, T: JobQueueOps + Send + Sync} ListArcSafe<0> for Dependency<'b, '_, T> { untracked; }
+}
+impl_list_item! {
+    impl{'b, T: JobQueueOps + Send + Sync} ListItem<0> for Dependency<'b, '_, T> {
+        using ListLinks { self.links }; }
+}
+
+#[pin_data]
+#[repr(C)]
+struct JobInternal<'b, T: JobQueueOps + Send + Sync> {
+    #[pin]
+    links: ListLinks,
+    cost: u32,
+    fence: Option<DriverFence<'b, DriverDataWrapper<T>>>,
+    dependencies: UniqueList<Dependency<'b, 'nr_of_deps, T>>,
+    nr_of_deps: Atomic<u32>,
+    jq: &'b JobQueue<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 }; }
+}
+
+struct SubmitWorker<'a, 'fctx, T: JobQueueOps + Send + Sync> {
+    srcu: &'a Srcu,
+    revoked: &'a AtomicFlag,
+    inner: &'a SpinLockIrq<JobQueueInner<'fctx, T>>,
+    fctx: &'fctx FenceContext<DriverDataWrapper<T>>,
+}
+
+impl<T: JobQueueOps + Send + Sync> ScopedWorkItem for SubmitWorker<'_, '_, T> {
+    fn run(work: &ScopedWorkRef<Self>) {
+        // Synchronize against revocation in `JobQueue::drop`.
+        let _lock_guard = work.srcu.read_lock();
+        if work.revoked.load(Relaxed) {
+            return;
+        }
+
+        let mut jq = work.inner.lock();
+
+        while let Some(job) = jq.as_mut().project().waiting_jobs.pop_front() {
+            // While we didn't hold the lock below for `run_job()`, it could be
+            // that a new job entered the waiting list which has an unfulfilled
+            // dependency.
+            if job.nr_of_deps.load(Relaxed) > 0 {
+                jq.waiting_jobs.push_front(job);
+                break;
+            }
+            let new_capacity = jq.capacity as i64 - i64::from(job.cost);
+            if new_capacity < 0 {
+                jq.waiting_jobs.push_front(job);
+                break;
+            }
+            *jq.as_mut().project().capacity = new_capacity as u64;
+
+            let fence = job.fence.as_ref().expect("fence not present");
+            let seqno = fence.as_fence().seqno();
+            let runnable_job = JobRunnable {
+                seqno,
+                data: fence.deref(),
+            };
+
+            drop(jq); // Release lock.
+            T::run_job(work.fctx, runnable_job);
+            jq = work.inner.lock(); // Retake lock.
+
+            // run_job() above needs to be called without lock protection. Thus,
+            // it is possible that the driver instructed completion right before
+            // we could re-take the lock.
+            if jq.last_completed >= seqno {
+                fence.signal(Ok(()));
+
+                // Drop here directly if possible. Otherwise, let the DropWorker
+                // take care of it.
+                if core::mem::needs_drop::<T::JobDataType>() {
+                    jq.drop_list.push_back(job);
+                }
+            } else {
+                jq.running_jobs.push_back(job);
+            }
+        }
+    }
+}
+
+struct DropWorker<'a, 'fctx, T: JobQueueOps + Send + Sync> {
+    srcu: &'a Srcu,
+    revoked: &'a AtomicFlag,
+    inner: &'a SpinLockIrq<JobQueueInner<'fctx, T>>,
+}
+
+impl<T: JobQueueOps + Send + Sync> ScopedWorkItem for DropWorker<'_, '_, T> {
+    fn run(work: &ScopedWorkRef<Self>) {
+        // Synchronize against revocation in `JobQueue::drop`.
+        let _lock_guard = work.srcu.read_lock();
+        if work.revoked.load(Relaxed) {
+            return;
+        }
+
+        let mut jq_locked = work.inner.lock();
+
+        while let Some(job) = jq_locked.drop_list.pop_front() {
+            drop(jq_locked);
+            drop(job);
+            jq_locked = work.inner.lock();
+        }
+    }
+}
+
+#[pin_data]
+struct JobQueueInner<'a, T: JobQueueOps + Send + Sync> {
+    /// Fence sequence number of the last job the user has completed.
+    #[pin]
+    /// List of jobs waiting to be run on the hardware.
+    waiting_jobs: UniqueList<JobInternal<'a, T>>,
+    #[pin]
+    /// Jobs that are currently executing on the hardware.
+    running_jobs: UniqueList<JobInternal<'a, T>>,
+    /// The queue's capacity.
+    capacity: u64,
+    /// The sequence number of the last completed job.
+    last_completed: u64,
+    #[pin]
+    /// Jobs that have finished executing and whose payload data `T` implements
+    /// `Drop`. These jobs are dropped in process context, which also allows for
+    /// their [`dma_buf::DriverFence`]s to implement the necessary RCU grace
+    /// period delay.
+    ///
+    /// If `Drop` is not implemented, this list is unused and the elements drop
+    /// directly in [`JobQueue::complete_jobs_up_to_seqno`].
+    ///
+    /// Should the JobQueue drop and the [`DropWorker`] not yet have dropped all
+    /// jobs, that is no problem because the JobQueue itself drops in process
+    /// context.
+    drop_list: UniqueList<JobInternal<'a, T>>,
+}
+
+/// A JobQueue.
+///
+/// When you drop your JobQueue, it might still attempt for one RCU grace period
+/// to submit more jobs via the [`JobQueueOps::run_job`] callback into your driver.
+///
+/// Dropping a JobQueue results in all remaining waiting and running jobs'
+/// fences being signaled with ECANCELED.
+///
+/// A JobQueue must only ever drop in non-atomic / process context.
+///
+/// # 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 FenceContextData {}
+///
+/// impl FenceContextData {
+///     fn new() -> impl PinInit<Self> {
+///         pin_init!(Self {})
+///     }
+/// }
+///
+/// impl FenceContextOps for FenceContextData {
+///     type FenceDataType = ();
+/// }
+///
+/// let fctx_data = FenceContextData::new();
+///
+/// let fctx = KBox::pin_init(
+///     FenceContext::new(0, c"dependency_driver", c"dependency_timeline", fctx_data),
+///     GFP_KERNEL
+/// )?;
+///
+/// let dependency1 = fctx.new_fence_allocation(())?.new_fence();
+/// let dependency2 = fctx.new_fence_allocation(())?.new_fence();
+///
+/// #[pin_data]
+/// struct JqData {}
+///
+/// impl JqData {
+///     fn new() -> impl PinInit<Self> {
+///         pin_init!(Self {})
+///     }
+/// }
+///
+/// struct Payload { }
+///
+/// impl JobQueueOps for JqData {
+///     type JobDataType = Payload;
+///
+///     fn run_job<'a>(&self, job: JobRunnable<'a, Payload>) {
+///         pr_info!("Job Nr. {} gets run now.\n", job.seqno);
+///     }
+///
+///     fn kill(&self, jq_nr: u64, err: Error) {
+///         pr_err!("The JQ Nr. {} became blocked forever because deps are blocked with err: {}\n",
+///                 jq_nr, err);
+///     }
+/// }
+///
+/// let jq_data = JqData::new();
+/// let jq = KBox::pin_init(
+///     JobQueue::new(0, driver_name, timeline_name, 1000, jq_data),
+///     GFP_KERNEL
+/// )?;
+///
+///
+/// let mut job1 = jq.new_job(1, Payload { })?;
+/// let mut job2 = jq.new_job(1, Payload { })?;
+///
+/// job1.add_dependency(dependency1.as_fence())?;
+/// job1.add_dependency(dependency1.as_fence())?;
+///
+/// job2.add_dependency(dependency2.as_fence())?;
+///
+/// struct Callback {
+///     seqno: u64,
+/// }
+///
+/// impl FenceCallback for Callback {
+///     fn on_signal(&mut self, res: Result) {
+///         pr_info!("Job Nr. {} got completed.\n", self.seqno);
+///     }
+/// }
+///
+/// let fence1 = job1.submit();
+/// let fence2 = job2.submit();
+///
+/// let cb = Callback { seqno: fence2.seqno() };
+/// // SAFETY: The registration is not leaked.
+/// let reg = unsafe { FenceCallbackRegistration::new(&fence2, cb) };
+/// let reg = KBox::new(reg, GFP_KERNEL)?;
+///
+/// dependency1.signal(Ok(()));
+/// dependency2.signal(Ok(()));
+///
+/// Ok::<(), Error>(())
+/// ```
+#[pin_data(PinnedDrop)]
+pub struct JobQueue<T: JobQueueOps + Send + Sync> {
+    #[pin]
+    #[uses('_: invariant, 'revoked: invariant, 'srcu: invariant)]
+    drop_worker: ScopedWork<DropWorker<'inner, 'fctx, T>>,
+    #[pin]
+    #[uses('_: invariant)]
+    submit_worker: ScopedWork<SubmitWorker<'inner, 'fctx, T>>,
+    #[pin]
+    #[uses('fctx: invariant)]
+    inner: SpinLockIrq<JobQueueInner<'fctx, T>>,
+    #[pin]
+    fctx: FenceContext<DriverDataWrapper<T>>,
+    revoked: AtomicFlag,
+    #[pin]
+    srcu: Srcu,
+}
+
+// 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.
+    ///
+    /// JobQueue calls this callback from process context without any locks
+    /// held, thus, the driver is permitted to sleep in its implementation.
+    ///
+    /// The driver must be aware that JobQueue trusts on its credit count system
+    /// for this operation to be infallible.
+    ///
+    /// This callback might be called while the driver is in the process of
+    /// dropping the JobQueue, but will never be called afterwards.
+    fn run_job<'a>(&self, job: JobRunnable<'a, Self::JobDataType>);
+
+    /// Tells the driver to tear this JobQueue down.
+    ///
+    /// The JobQueue backend invokes this in case a job's dependency got
+    /// signaled with an error. This means that the dependency's associated
+    /// action on a GPU ring have failed and will not be recovered.
+    ///
+    /// Thus, it is necessary to tear this JobQueue down, too. Upon receiving this
+    /// ops, the driver should stop submitting to this JobQueue and then call
+    /// [`JobQueue::teardown`] from a different context.
+    ///
+    /// This ops is invoked in atomic context.
+    fn kill(&self, jq_number: u64, err: Error);
+}
+
+#[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<T: JobQueueOps + Send + Sync> Deref for JobQueue<T> {
+    type Target = T;
+
+    fn deref(&self) -> &Self::Target {
+        &self.fctx.deref()
+    }
+}
+impl<T: JobQueueOps + Send + Sync> JobQueue<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,
+        capacity: u64,
+        data: impl PinInit<T, E>,
+    ) -> impl PinInit<Self, Error>
+    where
+        Error: From<E>,
+        E: From<Infallible>,
+    {
+        try_pin_init!(Self {
+            srcu <- new_srcu!(),
+            revoked: AtomicFlag::new(false),
+            fctx <- FenceContext::new(initial_seqno, &driver_name, &timeline_name,
+                                      DriverDataWrapper::new(data)),
+            inner <- new_spinlock_irq!(try_pin_init!(JobQueueInner {
+                capacity,
+                last_completed: 0,
+                waiting_jobs: UniqueList::new(),
+                running_jobs: UniqueList::new(),
+                drop_list: UniqueList::new(),
+            })),
+            submit_worker <- new_scoped_work!("JobQueueSubmitWorker", Ok(SubmitWorker {
+                srcu: Pin::get_ref(srcu),
+                revoked,
+                inner: Pin::get_ref(inner),
+                fctx: Pin::get_ref(fctx)
+            })),
+            drop_worker <- new_scoped_work!("JobQueueDropWorker", Ok(DropWorker {
+                srcu: Pin::get_ref(srcu),
+                revoked,
+                inner: Pin::get_ref(inner)
+            })),
+        })
+    }
+
+    /// Give the uniqe identifier for this JobQueue.
+    ///
+    /// This corresponds to the JobQueue's underlying [`dma_buf::FenceContext`].
+    pub fn id(&self) -> u64 {
+        self.fctx.id()
+    }
+
+    /// Create a new job.
+    ///
+    /// The `cost` specified here is what JobQueue subtracts from its capacity
+    /// to determine whether a job is runnable, i.e., has space on the GPU ring.
+    ///
+    /// The `data` contained by a job will be borrowed back to the driver once
+    /// the jobs shall be run through [`JobQueueOps::run_job`].
+    pub fn new_job<'a>(&'a self, cost: u32, 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(),
+            cost,
+            fence: None,
+            nr_of_deps: Atomic::new(0),
+            dependencies: UniqueList::new(),
+            jq: &self,
+        });
+
+        let inner = UniqueArc::pin_init(job, GFP_KERNEL)?;
+
+        Ok(Job {
+            inner,
+            fence_allocation,
+        })
+    }
+
+    fn check_start_submit_worker(&self) {
+        self.with_submit_worker(|submit_worker| {
+            // TODO this shouldn't be the system wq
+            // SAFETY: The worker is carried by the JobQueue and, thus, cannot be
+            // forgotten.
+            let _ = unsafe { workqueue::system_dfl().enqueue_scoped(submit_worker) };
+        })
+    }
+
+    fn check_start_drop_worker(&self) {
+        self.with_drop_worker(|drop_worker| {
+            // TODO this shouldn't be the system wq
+            // SAFETY: The worker is carried by the JobQueue and, thus, cannot be
+            // forgotten.
+            let _ = unsafe { workqueue::system_dfl().enqueue_scoped(drop_worker) };
+        })
+    }
+
+    /// Complete all the jobs in the queue's running_list, up to `seqno`.
+    ///
+    /// This signals the jobs' fences and schedules dropping of both the [`Job`]
+    /// and its [`DriverFence`].
+    ///
+    /// This signals the associated fences with the `Ok(())` status.
+    ///
+    /// This function tells the implementation that there is now more capacity
+    /// for more jobs to be pushed into the GPU ring.
+    pub fn complete_jobs_up_to_seqno(&self, seqno: u64) {
+        self.with_inner(|inner| {
+            let needs_drop = core::mem::needs_drop::<T::JobDataType>();
+            let mut jq_locked = inner.lock();
+            let mut jq = jq_locked.as_mut().project();
+
+            if seqno < *jq.last_completed {
+                return;
+            }
+
+            while let Some(mut job) = jq.running_jobs.pop_front() {
+                if job
+                    .as_mut()
+                    .project()
+                    .fence
+                    .as_mut()
+                    .expect("fence not present")
+                    .as_fence()
+                    .seqno()
+                    > seqno
+                {
+                    jq.running_jobs.push_front(job);
+                    break;
+                }
+
+                *jq.capacity += u64::from(job.cost);
+
+                let fence = job.fence.as_ref().expect("fence not present");
+                fence.signal(Ok(()));
+
+                if needs_drop {
+                    jq.drop_list.push_back(job);
+                }
+                // Otherwise, job is dropped here directly. Its fence won't perform
+                // an RCU grace period wait.
+            }
+
+            // This enables the SubmitWorker to complete a job that is not yet
+            // in 'running_jobs', in case of a race against the lock.
+            *jq.last_completed = seqno;
+            drop(jq_locked);
+
+            self.check_start_submit_worker();
+            if needs_drop {
+                self.check_start_drop_worker();
+            }
+        })
+    }
+
+    /// Tear the JobQueue down. Complete all pending jobs and signal their
+    /// fences with `error`.
+    ///
+    /// Only use this if you want to tear the JobQueue down in an error path and
+    /// want to specify the error.
+    ///
+    /// You can also just drop the JobQueue, which would result in all remaining
+    /// jobs' fences being signaled with ECANCELED.
+    ///
+    /// Note that this function has a small race window against the
+    /// `SubmitWorker` and its [`JobQueueOps::run_job`] callback, where the
+    /// latter might theoretically succeed at pushing more jobs to the driver.
+    /// The driver likely will want to ignore such attempts to push more jobs to
+    /// the GPU ring. Besides, the race merely results in these subsequent
+    /// jobs being signaled with ECANCELED on drop, thus, maybe differing from
+    /// the `error` provided here.
+    ///
+    /// This function must be called from non-atomic context.
+    pub fn teardown<E>(self, error: Error)
+    where
+        Error: From<E>,
+    {
+        self.with_inner(|inner| {
+            let mut jq = inner.lock();
+
+            // We don't drop the jobs and fences here while we hold the lock. Just
+            // signal with the error. The dropping lists will then drop them locklessly.
+
+            while let Some(job) = jq.running_jobs.pop_front() {
+                let fence = job.fence.as_ref().expect("fence not present");
+                fence.signal(Err(error));
+
+                jq.drop_list.push_back(job);
+            }
+        })
+    }
+}
+
+#[pinned_drop]
+impl<T: JobQueueOps + Send + Sync> PinnedDrop for JobQueue<T> {
+    fn drop(self: Pin<&mut Self>) {
+        // Revoke to prevent still existing DependencyWakers from kicking the
+        // SubmitWorker again before we could deregister them.
+        self.revoked.store(true, Relaxed);
+        self.srcu.synchronize();
+        self.with_inner(|inner| {
+            let mut inner = inner.lock();
+            let waiting_jobs = core::mem::take(&mut inner.waiting_jobs);
+            let running_jobs = core::mem::take(&mut inner.running_jobs);
+            let drop_list = core::mem::take(&mut inner.drop_list);
+            drop(inner);
+            drop(waiting_jobs);
+            drop(running_jobs);
+            drop(drop_list);
+        })
+    }
+}
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.56.0

[RFC PATCH v7 8/8] samples: rust: Add JobQueue Tester

[Philipp Stanner]

From: Gary Guo <gary@garyguo.net>

Some test code for the JobQueue
---
 samples/rust/Kconfig                 |  11 ++
 samples/rust/Makefile                |   1 +
 samples/rust/rust_jobqueue_tester.rs | 242 +++++++++++++++++++++++++++
 3 files changed, 254 insertions(+)
 create mode 100644 samples/rust/rust_jobqueue_tester.rs

diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
index 31d62533ef25..a5754fe03fd5 100644
--- a/samples/rust/Kconfig
+++ b/samples/rust/Kconfig
@@ -190,4 +190,15 @@ config SAMPLE_RUST_HOSTPROGS
 
 	  If unsure, say N.
 
+config SAMPLE_RUST_JOBQUEUE_TESTER
+	tristate "Jobqueue Tester"
+	select JOBQUEUE_TESTER
+	help
+	  This option builds the Rust Jobqueue Tester.
+
+	  To compile this as a module, choose M here:
+	  the module will be called rust_jobqueue_tester.
+
+	  If unsure, say N.
+
 endif # SAMPLES_RUST
diff --git a/samples/rust/Makefile b/samples/rust/Makefile
index b986b681cde5..69dd0b8d2597 100644
--- a/samples/rust/Makefile
+++ b/samples/rust/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_SAMPLE_RUST_DRIVER_AUXILIARY)	+= rust_driver_auxiliary.o
 obj-$(CONFIG_SAMPLE_RUST_DRIVER_SERDEV)		+= rust_driver_serdev.o
 obj-$(CONFIG_SAMPLE_RUST_CONFIGFS)		+= rust_configfs.o
 obj-$(CONFIG_SAMPLE_RUST_SOC)			+= rust_soc.o
+obj-$(CONFIG_SAMPLE_RUST_JOBQUEUE_TESTER)	+= rust_jobqueue_tester.o
 
 rust_print-y := rust_print_main.o rust_print_events.o
 
diff --git a/samples/rust/rust_jobqueue_tester.rs b/samples/rust/rust_jobqueue_tester.rs
new file mode 100644
index 000000000000..48dd1188f7db
--- /dev/null
+++ b/samples/rust/rust_jobqueue_tester.rs
@@ -0,0 +1,242 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Small example demonstrating how to use [`drm::JobQueue`].
+
+use kernel::dma_buf::{FenceContext, FenceContextOps, Fence, FenceStatus};
+use kernel::drm::{JobQueue, JobQueueOps, JobRunnable};
+use kernel::prelude::*;
+use kernel::str::CString;
+use kernel::task::might_sleep;
+use kernel::time::{delay::fsleep, Delta};
+
+module! {
+    type: RustJobqueueTester,
+    name: "rust_jobqueue_tester",
+    authors: ["Philipp Stanner"],
+    description: "Rust minimal sample",
+    license: "GPL",
+}
+
+// -- FenceContext data for external dependency fences -------------------------
+
+#[pin_data]
+struct DepFenceCtxData {}
+
+impl DepFenceCtxData {
+    fn new() -> impl PinInit<Self> {
+        pin_init!(Self {})
+    }
+}
+
+impl FenceContextOps for DepFenceCtxData {
+    type FenceDataType = ();
+}
+
+// -- JobQueue data and ops ----------------------------------------------------
+
+#[pin_data]
+struct JqData {}
+
+impl JqData {
+    fn new() -> impl PinInit<Self> {
+        pin_init!(Self {})
+    }
+}
+
+struct JobPayload {}
+
+impl Drop for JobPayload {
+    fn drop(&mut self) {
+        might_sleep();
+    }
+}
+
+impl JobQueueOps for JqData {
+    type JobDataType = JobPayload;
+
+    fn run_job<'a>(&self, job: JobRunnable<'a, JobPayload>) {
+        // Simulate GPU work completing after a short delay. In a real driver
+        // this would submit work to the hardware ring.
+        pr_info!("run_job: seqno {}\n", job.seqno);
+    }
+
+    fn kill(&self, _jq_nr: u64, _err: Error) {
+
+    }
+}
+
+fn is_signaled(fence: &Fence) -> bool {
+    match fence.status() {
+        FenceStatus::Unsignaled => false,
+        FenceStatus::Signaled(_) => true,
+    }
+}
+
+struct RustJobqueueTester {}
+
+impl kernel::Module for RustJobqueueTester {
+    fn init(_module: &'static ThisModule) -> Result<Self> {
+        pr_info!("Rust Jobqueue Tester (init)\n");
+        pr_info!("Am I built-in? {}\n", !cfg!(MODULE));
+
+        // A standalone FenceContext for creating external dependency fences.
+        let dep_fctx = KBox::pin_init(
+            FenceContext::new(
+                0,
+                c"rust-jq-tester",
+                c"dep-timeline",
+                DepFenceCtxData::new(),
+            ),
+            GFP_KERNEL,
+        )?;
+
+        let driver_name = CString::try_from_fmt(fmt!("rust-jobqueue-tester"))?;
+        let timeline_name = CString::try_from_fmt(fmt!("rust-jq-tester-hw-timeline"))?;
+        let jq = KBox::pin_init(
+            JobQueue::new(0, driver_name, timeline_name, 1_000_000, JqData::new()),
+            GFP_KERNEL,
+        )?;
+
+        pr_info!("Test 1: Test submitting two jobs without dependencies.\n");
+        let job1 = jq.new_job(1, JobPayload {})?;
+        let job2 = jq.new_job(1, JobPayload {})?;
+
+        let fence1 = job1.submit();
+        let fence2 = job2.submit();
+
+        // Complete both jobs by seqno so their fences get signaled.
+        jq.complete_jobs_up_to_seqno(fence2.seqno());
+
+        // Give the submit worker a chance to run.
+        fsleep(Delta::from_millis(100));
+
+        if !is_signaled(&fence1) || !is_signaled(&fence2) {
+            pr_info!("Test 1 failed.\n");
+            return Err(EAGAIN);
+        }
+        pr_info!("Test 1 succeeded.\n");
+
+        pr_info!("Test 2: Test submitting a job with already-fulfilled dependency.\n");
+        let mut job3 = jq.new_job(1, JobPayload {})?;
+        job3.add_dependency(&fence1)?;
+
+        let fence3 = job3.submit();
+        jq.complete_jobs_up_to_seqno(fence3.seqno());
+
+        // Give the submit worker a chance to run.
+        fsleep(Delta::from_millis(100));
+
+        if !is_signaled(&fence3) {
+            pr_info!("Test 2 failed.\n");
+            return Err(EAGAIN);
+        }
+        pr_info!("Test 2 succeeded.\n");
+
+        pr_info!("Test 3: Test that a job with unfulfilled dependency does not get run.\n");
+        let unsignaled_alloc = dep_fctx.new_fence_allocation(())?;
+        let unsignaled_fence = unsignaled_alloc.new_fence();
+
+        let mut job4 = jq.new_job(1, JobPayload {})?;
+        job4.add_dependency(unsignaled_fence.as_fence())?;
+
+        let blocked_job_fence = job4.submit();
+
+        // Give the submit worker a chance to run (it should NOT run job4).
+        fsleep(Delta::from_millis(100));
+
+        if is_signaled(&blocked_job_fence) {
+            pr_info!("Test 3 failed.\n");
+            return Err(EAGAIN);
+        }
+        pr_info!("Test 3 succeeded.\n");
+
+        pr_info!("Test 4: Test whether Test 3's blocked job can be unblocked.\n");
+        unsignaled_fence.signal(Ok(()));
+
+        // Give the submit worker a chance to run the now-unblocked job.
+        fsleep(Delta::from_millis(500));
+
+        // Complete it.
+        jq.complete_jobs_up_to_seqno(blocked_job_fence.seqno());
+        fsleep(Delta::from_millis(100));
+
+        if !is_signaled(&blocked_job_fence) {
+            pr_info!("Test 4 failed.\n");
+            return Err(EAGAIN);
+        }
+        pr_info!("Test 4 succeeded.\n");
+
+        pr_info!("Test 5: Submit unblocked jobs, then a blocked one, then unblocked ones.\n");
+        let job1 = jq.new_job(1, JobPayload {})?;
+        let job2 = jq.new_job(1, JobPayload {})?;
+        let mut job3 = jq.new_job(1, JobPayload {})?;
+        let job4 = jq.new_job(1, JobPayload {})?;
+        let job5 = jq.new_job(1, JobPayload {})?;
+
+        let unsignaled_alloc1 = dep_fctx.new_fence_allocation(())?;
+        let unsignaled_fence1 = unsignaled_alloc1.new_fence();
+        let unsignaled_alloc2 = dep_fctx.new_fence_allocation(())?;
+        let unsignaled_fence2 = unsignaled_alloc2.new_fence();
+        let unsignaled_alloc3 = dep_fctx.new_fence_allocation(())?;
+        let unsignaled_fence3 = unsignaled_alloc3.new_fence();
+
+        job3.add_dependency(unsignaled_fence1.as_fence())?;
+        job3.add_dependency(unsignaled_fence2.as_fence())?;
+        job3.add_dependency(unsignaled_fence3.as_fence())?;
+
+        let fence1 = job1.submit();
+        let fence2 = job2.submit();
+        let fence3 = job3.submit();
+
+        // Complete unblocked jobs 1 and 2.
+        jq.complete_jobs_up_to_seqno(fence2.seqno());
+        fsleep(Delta::from_millis(100));
+
+        if is_signaled(&fence3) || !is_signaled(&fence1) || !is_signaled(&fence2) {
+            pr_info!("Test 5 failed (phase 1).\n");
+            return Err(EAGAIN);
+        }
+
+        // Signal two of three dependencies — job3 should still be blocked.
+        unsignaled_fence1.signal(Ok(()));
+        unsignaled_fence3.signal(Ok(()));
+        fsleep(Delta::from_millis(200));
+
+        if is_signaled(&fence3) {
+            pr_info!("Test 5 failed (phase 2).\n");
+            return Err(EAGAIN);
+        }
+
+        // Signal the last dependency — job3 should now be runnable.
+        unsignaled_fence2.signal(Ok(()));
+        fsleep(Delta::from_millis(500));
+
+        // Complete job3 and the remaining jobs.
+        jq.complete_jobs_up_to_seqno(fence3.seqno());
+        fsleep(Delta::from_millis(100));
+
+        if !is_signaled(&fence3) {
+            pr_info!("Test 5 failed (phase 3).\n");
+            return Err(EAGAIN);
+        }
+
+        let fence4 = job4.submit();
+        let fence5 = job5.submit();
+        jq.complete_jobs_up_to_seqno(fence5.seqno());
+        fsleep(Delta::from_millis(100));
+
+        if !is_signaled(&fence4) || !is_signaled(&fence5) {
+            pr_info!("Test 5 failed (phase 4).\n");
+            return Err(EAGAIN);
+        }
+        pr_info!("Test 5 succeeded.\n");
+
+        Ok(RustJobqueueTester {})
+    }
+}
+
+impl Drop for RustJobqueueTester {
+    fn drop(&mut self) {
+        pr_info!("Rust Jobqueue Tester (exit)\n");
+    }
+}
-- 
2.56.0

Re: [RFC PATCH v7 4/8] rust: DmaFence: Replace call_rcu() with synchronize_rcu()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] rust: dma_fence: payload leak in DriverFence::drop() dropping NonNull wrapper
--

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

rust: DmaFence: Replace call_rcu() with synchronize_rcu()

This commit replaces the deferred RCU callback in DriverFence::drop() with
synchronize_rcu() so that driver fence data is not dropped from atomic
context, and updates FenceContext::drop() to use synchronize_rcu() instead
of rcu_barrier().

> diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
> index 735c4f090a2c4..fbbc6ab202eca 100644
> --- a/rust/kernel/dma_buf/dma_fence.rs
> +++ b/rust/kernel/dma_buf/dma_fence.rs

[ ... ]

> @@ -980,59 +968,18 @@ fn drop(&mut self) {
>              return;
>          }
>  
> +        // Make sure none of the fence backend_ops can access fence-data anymore.
> +        //
> +        // TODO:
> +        // This would not be necessary if the C dma_fence backend were using a
> +        // spinlock to properly synchronize its signaled state. Fix it in C and
> +        // then remove synchronize_rcu().
> +        synchronize_rcu();
> +
>          // SAFETY: Valid because `self` is valid.
> -        let rcu_head_ptr = unsafe { &raw mut (*self.data.as_ptr()).rcu_head };
> +        unsafe { drop_in_place(&raw mut self.data) };
>  
> -        // SAFETY: `call_rcu()` is always safe to be called. `rcu_head_ptr` was
> -        // created validly above. The module must perform a `synchronize_rcu()`
> -        // or `rcu_barrier()` call to guard against module unload.
> -        unsafe { bindings::call_rcu(rcu_head_ptr, Some(drop_driver_fence_data::<T>)) };
> +        // SAFETY: The `synchronize_rcu()` above ensures all accessors are gone.
> +        unsafe { bindings::dma_fence_put(self.as_raw()) };
>      }
>  }

[Severity: High]
As reported by Sashiko on RFC v6 [1], does DriverFence::drop() leak the
T::FenceDataType payload when a DriverFence is dropped?

In DriverFence, self.data has type NonNull<DriverFenceData<'a, T>>, while
the user payload of type T::FenceDataType lives in the inner data field of
DriverFenceData:

rust/kernel/dma_buf/dma_fence.rs:
    struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> {
        #[pin]
        inner: Fence,
        fctx: &'a FenceContext<T>,
        #[pin]
        data: T::FenceDataType,
    }
    ...
    pub struct DriverFence<'a, T: Send + Sync + FenceContextOps> {
        data: NonNull<DriverFenceData<'a, T>>,
    }

Because self is &mut DriverFence<'a, T>, passing &raw mut self.data to
drop_in_place() passes *mut NonNull<DriverFenceData<'a, T>> rather than
*mut T::FenceDataType. Since NonNull is a Copy pointer wrapper with no Drop
implementation, drop_in_place(&raw mut self.data) is a no-op, and the
subsequent dma_fence_put() releases the underlying allocation without ever
running the destructor for T::FenceDataType.

Should this obtain the raw dma_fence pointer before dropping the inner
payload, and then pass &raw mut (*self.data.as_ptr()).data to
drop_in_place() so that self.as_raw() does not create a shared reference to
DriverFenceData after its data field has been dropped in place?

[1] https://lore.kernel.org/r/20261016085724.1031766-6-phasta@kernel.org

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

Re: [RFC PATCH v7 5/8] rust: DmaFence: Replace is_signaled() with status()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] rust: dma_fence: unstable assert_matches in doctest breaks KUnit build
--

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

rust: DmaFence: Replace is_signaled() with status()

This commit replaces Fence::is_signaled() with Fence::status() returning a
FenceStatus enum so callers must check whether a signaled fence completed with
an error while holding the fence lock.

> diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
> index fbbc6ab202eca..9cef604f17ea9 100644
> --- a/rust/kernel/dma_buf/dma_fence.rs
> +++ b/rust/kernel/dma_buf/dma_fence.rs

[ ... ]

> @@ -650,11 +658,15 @@ struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> {
>  ///         FenceContextOps,
>  ///         FenceCallback,
>  ///         FenceCallbackRegistration,
> +///         FenceStatus, //
>  ///     },
>  ///     str::CString,
>  ///     sync::aref::ARef, //
>  /// };
> -/// use core::fmt::Display;
> +/// use core::{
> +///     assert_matches,
> +///     fmt::Display, //
> +/// };
>  ///
>  /// struct CallbackData { }
>  ///
> @@ -700,9 +712,9 @@ struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> {
>  /// let cb_reg = KBox::pin_init(cb_reg, GFP_KERNEL)?;
>  ///
>  /// // TODO signalling guards
> -/// assert_eq!(waiting_fence.is_signaled(), false);
> +/// assert_matches!(waiting_fence.status(), FenceStatus::Unsignaled);
>  /// fence.signal(Ok(()));
> -/// assert_eq!(waiting_fence.is_signaled(), true);
> +/// assert_matches!(waiting_fence.status(), FenceStatus::Signaled(Ok(())));

[Severity: Low]
Does using core::assert_matches in the DriverFence doctest break the build when
CONFIG_RUST_KERNEL_DOCTESTS=y and CONFIG_DMA_SHARED_BUFFER=y are enabled?

When CONFIG_RUST_KERNEL_DOCTESTS=y is set, scripts/rustdoc_test_gen extracts
this doctest into rust/doctests_kernel_generated.rs, which is compiled using
rust_common_cmd in scripts/Makefile.build:

    rust_allowed_features := arbitrary_self_types,asm_goto,generic_arg_infer,used_with_arg

Because assert_matches is an unstable library feature in core that is not in
rust_allowed_features and feature(assert_matches) is not enabled for the
doctest crate, compiling rust/doctests_kernel_generated.o fails with
error[E0658].

Would it work to use assert!(matches!(...)) instead, or derive PartialEq and Eq
on FenceStatus so assert_eq! can be used with the KUnit doctest assertion
macros?

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

Re: [RFC PATCH v7 6/8] rust: DmaFence: Tell results in signaling callbacks

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] rust: dma_fence: missing closing brace in DriverFence doctest
--

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

rust: DmaFence: Tell results in signaling callbacks

This commit updates the FenceCallback::on_signal() trait method in
rust/kernel/dma_buf/dma_fence.rs to pass the fence's signaling status as a
Result parameter to callback implementations.

> diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
> index 9cef604f17ea9..8c5d2a415504e 100644
> --- a/rust/kernel/dma_buf/dma_fence.rs
> +++ b/rust/kernel/dma_buf/dma_fence.rs

[ ... ]

> @@ -671,8 +681,12 @@ struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> {
>  /// struct CallbackData { }
>  ///
>  /// impl FenceCallback for CallbackData {
> -///     fn on_signal(&mut self) {
> -///         pr_info!("DmaFence callback executed.\n");
> +///     fn on_signal(&mut self, status: Result) {
> +///         if status.is_ok() {
> +///             pr_info!("DmaFence callback executed.\n");
> +///         else {
> +///             pr_err!("DmaFence callback executed, but fence has an error.\n");
> +///         }
>  ///     }
>  /// }

[Severity: Low]
In the DriverFence doc comment example in rust/kernel/dma_buf/dma_fence.rs,
is a closing brace missing before else in CallbackData::on_signal()? When
both CONFIG_DMA_SHARED_BUFFER and CONFIG_RUST_KERNEL_DOCTESTS are enabled,
building the kernel doctests fails with a syntax error because the if
status.is_ok() block is not closed.

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

Re: [RFC PATCH v7 8/8] samples: rust: Add JobQueue Tester

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] samples: rust: DriverFence drop order in init() sleeps under spinlock
- [High] samples: rust: premature job completion and seqno 0 leak queue capacity
- [High] rust: dma_fence: DriverFence::drop() drops NonNull instead of payload
- [Medium] samples: rust: broken Kconfig select and missing DRM=y dependency
- [Medium] samples: rust: vacuous blocked-job checks and late job4/5 submit in init()
- [Medium] rust: drm: DependencyWaker::on_signal() runs jobs after dependency error
- [Low] samples: rust: missing Signed-off-by and wrong module description
--

commit 8d8d7d10602aa63f4a1f88e3bb899d199cf28755
Author: Gary Guo <gary@garyguo.net>

samples: rust: Add JobQueue Tester

This commit adds a sample Rust module to test drm::JobQueue job submission,
dependency handling, and completion.

>     samples: rust: Add JobQueue Tester
>     
>     Some test code for the JobQueue

[Severity: Low]
This isn't a bug, but is the commit message missing a Signed-off-by tag?

> diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> index 31d62533ef25f..a5754fe03fd5c 100644
> --- a/samples/rust/Kconfig
> +++ b/samples/rust/Kconfig
> @@ -190,4 +190,15 @@ config SAMPLE_RUST_HOSTPROGS
>  
>  	  If unsure, say N.
>  
> +config SAMPLE_RUST_JOBQUEUE_TESTER
> +	tristate "Jobqueue Tester"
> +	select JOBQUEUE_TESTER

[Severity: Medium]
Does this Kconfig entry cause a Kconfig warning and build failure when
CONFIG_DRM is disabled or built as a module?

No Kconfig symbol named JOBQUEUE_TESTER exists in the tree, so selecting it
triggers an undefined symbol warning.

In addition, samples/rust/rust_jobqueue_tester.rs unconditionally imports
kernel::dma_buf and kernel::drm, which are gated in rust/kernel/lib.rs:

    #[cfg(CONFIG_DMA_SHARED_BUFFER)]
    pub mod dma_buf;
    pub mod driver;
    #[cfg(CONFIG_DRM = "y")]
    pub mod drm;

Should SAMPLE_RUST_JOBQUEUE_TESTER drop select JOBQUEUE_TESTER and add depends
on DRM=y?

Furthermore, when CONFIG_DRM=y is enabled, does rust/kernel/drm/job_queue.rs
compile cleanly? It currently imports new_scoped_work, ScopedWork,
ScopedWorkItem, and ScopedWorkRef and calls Queue::enqueue_scoped(), none of
which exist in rust/kernel/workqueue.rs, and implements FenceCallback (which
requires Send + 'static in rust/kernel/dma_buf/dma_fence.rs) for non-'static
DependencyWaker.

[ ... ]

> diff --git a/samples/rust/rust_jobqueue_tester.rs b/samples/rust/rust_jobqueue_tester.rs
> new file mode 100644
> index 0000000000000..48dd1188f7dbe
> --- /dev/null
> +++ b/samples/rust/rust_jobqueue_tester.rs
> @@ -0,0 +1,242 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Small example demonstrating how to use [`drm::JobQueue`].
> +
> +use kernel::dma_buf::{FenceContext, FenceContextOps, Fence, FenceStatus};
> +use kernel::drm::{JobQueue, JobQueueOps, JobRunnable};
> +use kernel::prelude::*;
> +use kernel::str::CString;
> +use kernel::task::might_sleep;
> +use kernel::time::{delay::fsleep, Delta};
> +
> +module! {
> +    type: RustJobqueueTester,
> +    name: "rust_jobqueue_tester",
> +    authors: ["Philipp Stanner"],
> +    description: "Rust minimal sample",
> +    license: "GPL",
> +}

[Severity: Low]
This isn't a bug, but was the description "Rust minimal sample" in module!
copied from samples/rust/rust_minimal.rs instead of describing the JobQueue
tester?

[ ... ]

> +struct JobPayload {}
> +
> +impl Drop for JobPayload {
> +    fn drop(&mut self) {
> +        might_sleep();
> +    }
> +}

[Severity: High]
Does JobPayload::drop() ever get invoked when jobs and their fences are
dropped?

JobQueue stores T::JobDataType (JobPayload) as the FenceDataType of its
internal FenceContext. In rust/kernel/dma_buf/dma_fence.rs, DriverFence holds
data: NonNull<DriverFenceData<'a, T>>, and DriverFence::drop() currently
runs:

    // SAFETY: Valid because `self` is valid.
    unsafe { drop_in_place(&raw mut self.data) };

Because self.data is a NonNull pointer wrapper with no Drop implementation,
drop_in_place(&raw mut self.data) is a no-op instead of dropping
(*self.data.as_ptr()).data. As a result, T::FenceDataType is leaked and the
sleepable drop test in JobPayload::drop() is bypassed.

> +
> +impl JobQueueOps for JqData {
> +    type JobDataType = JobPayload;
> +
> +    fn run_job<'a>(&self, job: JobRunnable<'a, JobPayload>) {
> +        // Simulate GPU work completing after a short delay. In a real driver
> +        // this would submit work to the hardware ring.
> +        pr_info!("run_job: seqno {}\n", job.seqno);
> +    }

[ ... ]

> +        let driver_name = CString::try_from_fmt(fmt!("rust-jobqueue-tester"))?;
> +        let timeline_name = CString::try_from_fmt(fmt!("rust-jq-tester-hw-timeline"))?;
> +        let jq = KBox::pin_init(
> +            JobQueue::new(0, driver_name, timeline_name, 1_000_000, JqData::new()),
> +            GFP_KERNEL,
> +        )?;
> +
> +        pr_info!("Test 1: Test submitting two jobs without dependencies.\n");
> +        let job1 = jq.new_job(1, JobPayload {})?;
> +        let job2 = jq.new_job(1, JobPayload {})?;
> +
> +        let fence1 = job1.submit();
> +        let fence2 = job2.submit();
> +
> +        // Complete both jobs by seqno so their fences get signaled.
> +        jq.complete_jobs_up_to_seqno(fence2.seqno());
> +
> +        // Give the submit worker a chance to run.
> +        fsleep(Delta::from_millis(100));

[Severity: High]
Does passing initial_seqno = 0 to JobQueue::new() and calling
complete_jobs_up_to_seqno() before sleeping cause premature job completion and
leak queue capacity?

First, in rust/kernel/drm/job_queue.rs, JobQueue::new() initializes
JobQueueInner::last_completed to 0:

    inner <- new_spinlock_irq!(try_pin_init!(JobQueueInner {
        capacity,
        last_completed: 0,
        waiting_jobs: UniqueList::new(),
        running_jobs: UniqueList::new(),
        drop_list: UniqueList::new(),
    })),

When initial_seqno is 0, FenceContext::next_seqno() assigns seqno 0 to the
first submitted job, which immediately satisfies jq.last_completed >= seqno
(0 >= 0) in SubmitWorker::run() even before complete_jobs_up_to_seqno() is
called.

Second, in Test 1, Test 2, and Test 5 (phases 1 and 4),
jq.complete_jobs_up_to_seqno() is called synchronously right after
job.submit() and before fsleep(). Because SubmitWorker runs asynchronously on
a workqueue, the jobs are still in waiting_jobs rather than running_jobs when
complete_jobs_up_to_seqno() executes, so complete_jobs_up_to_seqno() only
updates last_completed = seqno while running_jobs and drop_list are empty.

When SubmitWorker::run() later processes those jobs, it subtracts job.cost
from jq.capacity and takes the jq.last_completed >= seqno branch:

    if jq.last_completed >= seqno {
        fence.signal(Ok(()));

        // Drop here directly if possible. Otherwise, let the DropWorker
        // take care of it.
        if core::mem::needs_drop::<T::JobDataType>() {
            jq.drop_list.push_back(job);
        }
    } else {
        jq.running_jobs.push_back(job);
    }

Unlike complete_jobs_up_to_seqno(), that branch never restores jq.capacity +=
u64::from(job.cost), permanently leaking capacity credits, and never calls
check_start_drop_worker() after pushing onto jq.drop_list.

[ ... ]

> +        pr_info!("Test 3: Test that a job with unfulfilled dependency does not get run.\n");
> +        let unsignaled_alloc = dep_fctx.new_fence_allocation(())?;
> +        let unsignaled_fence = unsignaled_alloc.new_fence();
> +
> +        let mut job4 = jq.new_job(1, JobPayload {})?;
> +        job4.add_dependency(unsignaled_fence.as_fence())?;
> +
> +        let blocked_job_fence = job4.submit();
> +
> +        // Give the submit worker a chance to run (it should NOT run job4).
> +        fsleep(Delta::from_millis(100));
> +
> +        if is_signaled(&blocked_job_fence) {
> +            pr_info!("Test 3 failed.\n");
> +            return Err(EAGAIN);
> +        }

[Severity: High]
If RustJobqueueTester::init() returns an error here in Test 3 or later in
Test 5 while an unsignaled dependency DriverFence has a registered callback,
will stack unwinding sleep while holding a spinlock with interrupts disabled?

Because jq is declared before unsignaled_fence in Test 3 (and before job3 and
unsignaled_fence1, unsignaled_fence2, and unsignaled_fence3 in Test 5), Rust
drops the unsignaled DriverFence locals before job3 and jq on any early error
return.

When an unsignaled DriverFence is dropped, DriverFence::drop() in
rust/kernel/dma_buf/dma_fence.rs acquires dma_fence_lock_irqsave(), sets
ECANCELED, and calls dma_fence_signal_locked(). That synchronously invokes
DependencyWaker::on_signal() in rust/kernel/drm/job_queue.rs with
Err(ECANCELED):

    fn on_signal(&mut self, res: Result) {
        if let Err(err) = res {
            // Dependencies are unfulfilled and will never be fulfilled. Halt the
            // submit_worker and tell the driver to tear this JQ down.
            self.jq.revoked.store(true, Relaxed);
            self.jq.srcu.synchronize();

Calling self.jq.srcu.synchronize() (synchronize_srcu()) while holding
dma_fence_lock_irqsave sleeps in atomic context.

[Severity: Medium]
Also in DependencyWaker::on_signal(), when res is Err(err), execution does not
return after calling T::kill() and instead falls through to:

        // Last dependency kicks the worker.
        if self.nr_of_deps.fetch_sub(1, Relaxed) == 1 {
            // The drop-order of JobQueue ensures that all DependencyWakers are
            // cancelled before the JobQueue itself is dropped.
            self.jq.check_start_submit_worker();
        }

In SubmitWorker::run(), work.revoked.load(Relaxed) is only checked once before
the while let Some(job) = jq.as_mut().project().waiting_jobs.pop_front() loop,
and is not re-checked after retaking work.inner.lock() across T::run_job().
Could a failed dependency decrement nr_of_deps to 0 and allow SubmitWorker to
run a job whose dependency errored?

[ ... ]

> +        pr_info!("Test 5: Submit unblocked jobs, then a blocked one, then unblocked ones.\n");
> +        let job1 = jq.new_job(1, JobPayload {})?;
> +        let job2 = jq.new_job(1, JobPayload {})?;
> +        let mut job3 = jq.new_job(1, JobPayload {})?;
> +        let job4 = jq.new_job(1, JobPayload {})?;
> +        let job5 = jq.new_job(1, JobPayload {})?;
> +
> +        let unsignaled_alloc1 = dep_fctx.new_fence_allocation(())?;
> +        let unsignaled_fence1 = unsignaled_alloc1.new_fence();
> +        let unsignaled_alloc2 = dep_fctx.new_fence_allocation(())?;
> +        let unsignaled_fence2 = unsignaled_alloc2.new_fence();
> +        let unsignaled_alloc3 = dep_fctx.new_fence_allocation(())?;
> +        let unsignaled_fence3 = unsignaled_alloc3.new_fence();
> +
> +        job3.add_dependency(unsignaled_fence1.as_fence())?;
> +        job3.add_dependency(unsignaled_fence2.as_fence())?;
> +        job3.add_dependency(unsignaled_fence3.as_fence())?;
> +
> +        let fence1 = job1.submit();
> +        let fence2 = job2.submit();
> +        let fence3 = job3.submit();
> +
> +        // Complete unblocked jobs 1 and 2.
> +        jq.complete_jobs_up_to_seqno(fence2.seqno());
> +        fsleep(Delta::from_millis(100));
> +
> +        if is_signaled(&fence3) || !is_signaled(&fence1) || !is_signaled(&fence2) {
> +            pr_info!("Test 5 failed (phase 1).\n");
> +            return Err(EAGAIN);
> +        }
> +
> +        // Signal two of three dependencies — job3 should still be blocked.
> +        unsignaled_fence1.signal(Ok(()));
> +        unsignaled_fence3.signal(Ok(()));
> +        fsleep(Delta::from_millis(200));
> +
> +        if is_signaled(&fence3) {
> +            pr_info!("Test 5 failed (phase 2).\n");
> +            return Err(EAGAIN);
> +        }
> +
> +        // Signal the last dependency — job3 should now be runnable.
> +        unsignaled_fence2.signal(Ok(()));
> +        fsleep(Delta::from_millis(500));
> +
> +        // Complete job3 and the remaining jobs.
> +        jq.complete_jobs_up_to_seqno(fence3.seqno());
> +        fsleep(Delta::from_millis(100));
> +
> +        if !is_signaled(&fence3) {
> +            pr_info!("Test 5 failed (phase 3).\n");
> +            return Err(EAGAIN);
> +        }
> +
> +        let fence4 = job4.submit();
> +        let fence5 = job5.submit();

[Severity: Medium]
Are the blocked-job checks in Test 3 and Test 5 (phases 1 and 2) able to
detect if a blocked job is run prematurely, and should job4 and job5 be
submitted before job3 is unblocked?

In SubmitWorker::run(), running a job calls JqData::run_job() and moves the
job to running_jobs without signaling the job fence unless
jq.last_completed >= seqno. Because JqData::run_job() only prints a message
and complete_jobs_up_to_seqno() has not been called for blocked_job_fence in
Test 3 or fence3 in Test 5 phases 1 and 2, is_signaled(&blocked_job_fence) and
is_signaled(&fence3) will always return false even if SubmitWorker ignored
dependencies and called run_job() immediately.

Additionally, Test 5 states "Submit unblocked jobs, then a blocked one, then
unblocked ones" and creates job1 through job5 up front, with the phase 3
comment saying "Complete job3 and the remaining jobs." However, job4.submit()
and job5.submit() are only called here in phase 4 after job3 has already been
unblocked and completed, so job4 and job5 are never queued behind the blocked
job3.

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

Re: [RFC PATCH v7 7/8] rust: drm: Add JobQueue

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 13 potential issue(s) to consider:
- [High] drm: job_queue: sleeping synchronize_srcu() in DependencyWaker::on_signal()
- [High] drm: job_queue: jobs with failed dependencies run in SubmitWorker::run()
- [High] drm: job_queue: ignored fence error status in Job::add_dependency()
- [High] drm: job_queue: premature completion of seqno 0 job in SubmitWorker::run()
- [High] drm: job_queue: leaked capacity and unscheduled drop in SubmitWorker::run()
- [High] dma_buf: dma_fence: payload destructor skipped in DriverFence::drop()
- [High] rust: drm: job_queue: unsignaled waiting_jobs in teardown() and drop()
- [High] rust: drm: job_queue: fence signaling and job drop under SpinLockIrq
- [High] rust: drm: job_queue: oversized job cost permanently wedges FIFO queue
- [High] rust: drm: job_queue: aliasing &JobQueue in DependencyWaker during drop()
- [Medium] drm: job_queue: uncallable self receiver and missed jobs in teardown()
- [Medium] rust: drm: job_queue: signed i64 cast of u64 capacity in SubmitWorker::run()
- [Low] rust: drm: job_queue: commit message mismatch, doc links, and import style
--

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

rust: drm: Add JobQueue

This commit adds the JobQueue component in rust/kernel/drm/job_queue.rs to
manage job dependencies, credit-based submission, and dma_fence lifetimes
for GPUs with firmware scheduling.

>     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.
>     
>     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 fulfilled 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.
>     
>     The submit_worker runs all ready jobs based on a credit count system, so
>     that run_job() can be infallible. The same worker drops jobs (and their
>     fences) that are already completed. Should this proof to be a
>     performance issue, a second worker could trivially be added for this.
>     
>     Signed-off-by: Philipp Stanner <phasta@kernel.org>
>     Co-developed-by: Gary Guo <gary@garyguo.net>
>     
>     (^ waiting for Gary to sign-off)

[Severity: Low]
This isn't a bug, but does the commit message still match the current
implementation in rust/kernel/drm/job_queue.rs?

The commit message states that submit_worker drops completed jobs and that a
second worker could be added later, whereas the patch already implements and
uses a separate DropWorker (drop_worker). It also mentions that the code
cannot be used yet due to self-referential PinInit work and refers to
comments in the Example that are no longer present, and it still has the
placeholder note waiting for Gary Guo's sign-off.

In addition, in rust/kernel/drm/job_queue.rs:
- The module doc comment links to JobQueueOps::complete_jobs_up_to_seqno(),
  which is a method on JobQueue rather than JobQueueOps, as well as
  dma_buf::FenceContext, dma_buf::DriverFence, dma_buf::Fence, and
  rcu::synchronize_rcu() without dma_buf or rcu in scope.
- In struct JobQueueInner, a duplicate doc comment for last_completed
  ("Fence sequence number of the last job the user has completed.") is
  attached to waiting_jobs.
- The kernel::sync imports are split across three separate sync:: blocks
  instead of using the kernel vertical import style.

> diff --git a/rust/kernel/drm/job_queue.rs b/rust/kernel/drm/job_queue.rs
> new file mode 100644
> index 0000000000000..e51adedaeddbf
> --- /dev/null
> +++ b/rust/kernel/drm/job_queue.rs
[ ... ]
> @@ -142,29 +142,115 @@ struct DependencyWaker<'b, 'nr_of_deps, T: JobQueueOps + Send + Sync> {
>      nr_of_deps: &'nr_of_deps Atomic<u32>,
>      jq: &'b JobQueue<T>,
>  }
>  
>  impl<'b, T: JobQueueOps + Send + Sync> FenceCallback for DependencyWaker<'b, '_, T> {
>      fn on_signal(&mut self, res: Result) {
>          if let Err(err) = res {
>              // Dependencies are unfulfilled and will never be fulfilled. Halt the
>              // submit_worker and tell the driver to tear this JQ down.
>              self.jq.revoked.store(true, Relaxed);
>              self.jq.srcu.synchronize();
>  
>              // TODO RFC:
>              // We likely want a work_item for this, don't we? Because the
>              // dma_fence backend is holding the lock.
>              T::kill(self.jq.fctx.deref(), self.jq.id(), err);
>          }

[Severity: High]
Does calling self.jq.srcu.synchronize() in DependencyWaker::on_signal()
sleep in atomic context?

FenceCallback::on_signal() is invoked from
FenceCallbackRegistration::dma_fence_callback() during C
dma_fence_signal_locked() while holding the fence spinlock with interrupts
disabled, and potentially from hardware interrupt context.

When a dependency fence is signaled with Err(err), self.jq.srcu.synchronize()
calls bindings::synchronize_srcu() in Srcu::synchronize()
(rust/kernel/sync/srcu.rs), which sleeps waiting for an SRCU grace period to
complete. If SubmitWorker::run() or DropWorker::run() is currently holding
work.srcu.read_lock() (for example on the same CPU or while dropping a job),
won't this also deadlock waiting for the active SRCU read-side critical
section?

>  
>          // Last dependency kicks the worker.
>          if self.nr_of_deps.fetch_sub(1, Relaxed) == 1 {
>              // The drop-order of JobQueue ensures that all DependencyWakers are
>              // cancelled before the JobQueue itself is dropped.
>              self.jq.check_start_submit_worker();
>          }
>      }
>  }

[Severity: High]
When res is Err(err) in DependencyWaker::on_signal(), execution falls through
to decrement self.nr_of_deps and call self.jq.check_start_submit_worker().
Can this cause a job with a failed dependency to still be executed by
SubmitWorker::run()?

In SubmitWorker::run(), work.revoked.load(Relaxed) is only checked once
before entering the waiting_jobs loop:

    let _lock_guard = work.srcu.read_lock();
    if work.revoked.load(Relaxed) {
        return;
    }

    let mut jq = work.inner.lock();

    while let Some(job) = jq.as_mut().project().waiting_jobs.pop_front() {
        if job.nr_of_deps.load(Relaxed) > 0 {
            jq.waiting_jobs.push_front(job);
            break;
        }
        ...
        drop(jq); // Release lock.
        T::run_job(work.fctx, runnable_job);
        jq = work.inner.lock(); // Retake lock.

If SubmitWorker::run() is currently unlocked inside T::run_job() for an
earlier job when a subsequent job's dependency signals Err(err),
DependencyWaker::on_signal() decrements that job's nr_of_deps to 0. On the
next loop iteration, SubmitWorker::run() does not re-check work.revoked, sees
nr_of_deps == 0, and dispatches the job to T::run_job() despite its failed
dependency.

Also, because DependencyWaker::on_signal() does not check whether
self.jq.revoked was already set before calling T::kill(), can T::kill() be
invoked multiple times if multiple dependencies fail, while DropWorker::run()
exits early on work.revoked and leaves already-completed jobs in drop_list
undropped?

>  
>  /// 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: Pin<UniqueArc<JobInternal<'a, T>>>,
>      fence_allocation: DriverFenceAllocation<'a, DriverDataWrapper<T>>,
>  }
>  
>  impl<'a, T: JobQueueOps + Send + Sync> Job<'a, T> {
>      /// Add a dependency for the job.
>      ///
>      /// A JobQueue will only push a job once `fence` has been signaled.
>      pub fn add_dependency(&mut self, fence: &Fence) -> Result {
>          self.inner.as_mut().with_project(|job| {
>              // 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);
>  
>              let dependency = try_pin_init!(Dependency {
>                  links <- ListLinks::new(),
>                  // SAFETY: The `registration` is not forgotten. It drops with JobQueue.
>                  registration <- unsafe {
>                      FenceCallbackRegistration::new(
>                          fence,
>                          DependencyWaker {
>                              nr_of_deps: job.nr_of_deps,
>                              jq: job.jq,
>                          },
>                      )
>                  }
>              });
>              match UniqueArc::pin_init(dependency, GFP_KERNEL) {
>                  Err(e) => {
>                      let _ = job.nr_of_deps.fetch_sub(1, Relaxed);
>                      if e == ENOENT {
>                          // Was already signaled. No reason to bother the caller.
>                          return Ok(());
>                      }
>                      return Err(e);
>                  }

[Severity: High]
If a dependency fence has already been signaled with an error before
Job::add_dependency() is called, does this ENOENT branch silently ignore the
fence error?

In C dma_fence_add_callback(), -ENOENT is returned whenever
DMA_FENCE_FLAG_SIGNALED_BIT is set, even if fence->error is non-zero. Since
Job::add_dependency() returns Ok(()) on ENOENT without checking
fence.status(), won't a pre-signaled errored dependency fence allow the job
to run as if all dependencies succeeded?

>                  Ok(dep) => {
>                      job.dependencies.push_back(dep);
>                  }
>              }
>  
>              Ok(())
>          })
>      }
>  
>      /// Submit the 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.
>      ///
>      /// This function does all fence initialization under the JobQueue lock in
>      /// one go, including setting of the fence sequence number. Thus, it is
>      /// typically unnecessary (and advaised against) to call this function with
>      /// a driver lock held.
>      pub fn submit(self) -> ARef<Fence> {
>          let jq = self.inner.jq;
>          jq.with_inner(|inner| {
>              // SAFETY: this is a lifetime transmute from `Job<'queue, T>` to
>              // `Job<'fctx, T>`.
>              //
>              // `job` internally holds a reference to `JobQueue`, while we are
>              // now moving `job` into `inner`, which may borrow from `fctx` but
>              // not the full `JobQueue`. So on surface this would be incorrect.
>              // However, as we drain `inner` inside `JobQueue::drop`, the
>              // reference is gone before any field is destructed.
>              let mut job = unsafe { core::mem::transmute::<Job<'_, T>, Job<'_, T>>(self) };

[Severity: High]
Does erasing the borrow lifetime of self.inner.jq in Job::submit() create
aliasing between &JobQueue<T> in DependencyWaker::on_signal() and
Pin<&mut JobQueue<T>> in <JobQueue<T> as PinnedDrop>::drop()?

After Job::submit() transmutes the lifetime of Job<'_, T>, the caller can
drop JobQueue<T> while DependencyWaker callbacks in waiting_jobs are still
registered on external dma_fences:

    impl<T: JobQueueOps + Send + Sync> PinnedDrop for JobQueue<T> {
        fn drop(self: Pin<&mut Self>) {
            self.revoked.store(true, Relaxed);
            self.srcu.synchronize();
            self.with_inner(|inner| {
                let mut inner = inner.lock();
                let waiting_jobs = core::mem::take(&mut inner.waiting_jobs);
                ...
                drop(waiting_jobs);

Because DependencyWaker::on_signal() neither enters an SRCU read-side
critical section on self.jq.srcu nor checks self.jq.revoked before accessing
self.jq, a dependency fence signaling on another CPU before
drop(waiting_jobs) completes will dereference self.jq: &JobQueue<T>
concurrently while PinnedDrop::drop() holds an exclusive Pin<&mut Self>
reference.

[ ... ]
> @@ -303,75 +303,75 @@ impl<T: JobQueueOps + Send + Sync> ScopedWorkItem for SubmitWorker<'_, '_, T> {
>      fn run(work: &ScopedWorkRef<Self>) {
>          // Synchronize against revocation in `JobQueue::drop`.
>          let _lock_guard = work.srcu.read_lock();
>          if work.revoked.load(Relaxed) {
>              return;
>          }
>  
>          let mut jq = work.inner.lock();
>  
>          while let Some(job) = jq.as_mut().project().waiting_jobs.pop_front() {
>              // While we didn't hold the lock below for `run_job()`, it could be
>              // that a new job entered the waiting list which has an unfulfilled
>              // dependency.
>              if job.nr_of_deps.load(Relaxed) > 0 {
>                  jq.waiting_jobs.push_front(job);
>                  break;
>              }
>              let new_capacity = jq.capacity as i64 - i64::from(job.cost);
>              if new_capacity < 0 {
>                  jq.waiting_jobs.push_front(job);
>                  break;
>              }
>              *jq.as_mut().project().capacity = new_capacity as u64;

[Severity: Medium]
In SubmitWorker::run(), if JobQueue::new() is called with a capacity greater
than i64::MAX as u64, won't casting jq.capacity from u64 to i64 wrap into a
negative number (or overflow i64::MIN when subtracting job.cost) and stall
the queue? Would jq.capacity.checked_sub(u64::from(job.cost)) avoid the
signed cast?

>  
>              let fence = job.fence.as_ref().expect("fence not present");
>              let seqno = fence.as_fence().seqno();
>              let runnable_job = JobRunnable {
>                  seqno,
>                  data: fence.deref(),
>              };
>  
>              drop(jq); // Release lock.
>              T::run_job(work.fctx, runnable_job);
>              jq = work.inner.lock(); // Retake lock.
>  
>              // run_job() above needs to be called without lock protection. Thus,
>              // it is possible that the driver instructed completion right before
>              // we could re-take the lock.
>              if jq.last_completed >= seqno {
>                  fence.signal(Ok(()));
>  
>                  // Drop here directly if possible. Otherwise, let the DropWorker
>                  // take care of it.
>                  if core::mem::needs_drop::<T::JobDataType>() {
>                      jq.drop_list.push_back(job);
>                  }
>              } else {
>                  jq.running_jobs.push_back(job);
>              }
>          }
>      }
>  }

[Severity: High]
As reported by Sashiko on RFC v6 [1], when a job completes in
JobQueue::complete_jobs_up_to_seqno() before SubmitWorker::run() re-acquires
work.inner.lock() after T::run_job(), doesn't the if jq.last_completed >= seqno
branch leak job.cost from jq.capacity and fail to schedule drop_worker?

Before releasing work.inner.lock() to call T::run_job(), SubmitWorker::run()
subtracts job.cost from jq.capacity while the job is not in jq.running_jobs.
If JobQueue::complete_jobs_up_to_seqno() runs during that window, it does not
find the job in jq.running_jobs, so it only updates *jq.last_completed = seqno
without restoring job.cost to jq.capacity.

When SubmitWorker::run() retakes work.inner.lock() and enters
if jq.last_completed >= seqno, it signals the fence and pushes the job to
jq.drop_list without adding u64::from(job.cost) back to jq.capacity and
without kicking drop_worker.

>  
>  struct DropWorker<'a, 'fctx, T: JobQueueOps + Send + Sync> {
>      srcu: &'a Srcu,
>      revoked: &'a AtomicFlag,
>      inner: &'a SpinLockIrq<JobQueueInner<'fctx, T>>,
>  }
>  
>  impl<T: JobQueueOps + Send + Sync> ScopedWorkItem for DropWorker<'_, '_, T> {
>      fn run(work: &ScopedWorkRef<Self>) {
>          // Synchronize against revocation in `JobQueue::drop`.
>          let _lock_guard = work.srcu.read_lock();
>          if work.revoked.load(Relaxed) {
>              return;
>          }
>  
>          let mut jq_locked = work.inner.lock();
>  
>          while let Some(job) = jq_locked.drop_list.pop_front() {
>              drop(jq_locked);
>              drop(job);
>              jq_locked = work.inner.lock();
>          }
>      }
>  }

[Severity: High]
When DropWorker::run() (or JobQueue::drop()) drops a completed job whose
T::JobDataType implements Drop, is the destructor of T::JobDataType actually
executed by DriverFence::drop()?

In rust/kernel/dma_buf/dma_fence.rs, DriverFence<'a, T> stores self.data as
NonNull<DriverFenceData<'a, T>>, and DriverFence::drop() does:

    // SAFETY: Valid because `self` is valid.
    unsafe { drop_in_place(&raw mut self.data) };

    // SAFETY: The `synchronize_rcu()` above ensures all accessors are gone.
    unsafe { bindings::dma_fence_put(self.as_raw()) };

Because self.data is a NonNull pointer rather than the payload itself,
drop_in_place(&raw mut self.data) is a no-op and never runs the destructor of
(*self.data.as_ptr()).data (which holds T::JobDataType) before dma_fence_put()
frees the underlying allocation.

[ ... ]
> @@ -620,72 +620,148 @@ impl<T: JobQueueOps + Send + Sync> JobQueue<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,
>          capacity: u64,
>          data: impl PinInit<T, E>,
>      ) -> impl PinInit<Self, Error>
>      where
>          Error: From<E>,
>          E: From<Infallible>,
>      {
>          try_pin_init!(Self {
>              srcu <- new_srcu!(),
>              revoked: AtomicFlag::new(false),
>              fctx <- FenceContext::new(initial_seqno, &driver_name, &timeline_name,
>                                        DriverDataWrapper::new(data)),
>              inner <- new_spinlock_irq!(try_pin_init!(JobQueueInner {
>                  capacity,
>                  last_completed: 0,
>                  waiting_jobs: UniqueList::new(),
>                  running_jobs: UniqueList::new(),
>                  drop_list: UniqueList::new(),
>              })),

[Severity: High]
If JobQueue::new() is called with initial_seqno == 0 (as in the doctest
example), won't initializing last_completed: 0 cause SubmitWorker::run() to
treat the very first job (which receives seqno == 0 from
FenceContext::next_seqno()) as already completed immediately after
T::run_job() returns?

In SubmitWorker::run():

    if jq.last_completed >= seqno {
        fence.signal(Ok(()));

        if core::mem::needs_drop::<T::JobDataType>() {
            jq.drop_list.push_back(job);
        }
    } else {
        jq.running_jobs.push_back(job);
    }

When initial_seqno is 0, jq.last_completed >= seqno evaluates 0 >= 0 as true
for the first job before complete_jobs_up_to_seqno() has ever been called,
prematurely signaling its fence, dropping the job while it is still running
on hardware, and leaking its capacity credits.

[ ... ]
>      /// Create a new job.
>      ///
>      /// The `cost` specified here is what JobQueue subtracts from its capacity
>      /// to determine whether a job is runnable, i.e., has space on the GPU ring.
>      ///
>      /// The `data` contained by a job will be borrowed back to the driver once
>      /// the jobs shall be run through [`JobQueueOps::run_job`].
>      pub fn new_job<'a>(&'a self, cost: u32, 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(),
>              cost,
>              fence: None,
>              nr_of_deps: Atomic::new(0),
>              dependencies: UniqueList::new(),
>              jq: &self,
>          });

[Severity: High]
As reported by Sashiko on RFC v6 [2], if a job is created in
JobQueue::new_job() with a cost greater than the queue's total initial
capacity and then submitted, won't SubmitWorker::run() permanently wedge the
FIFO queue?

Because JobQueue::new_job() does not reject cost values exceeding the maximum
queue capacity, SubmitWorker::run() will always seejq.capacity < job.cost
even after all running jobs complete, pushing the oversized job back to the
front of waiting_jobs and blocking all subsequent jobs in the queue.

[ ... ]
>      pub fn complete_jobs_up_to_seqno(&self, seqno: u64) {
>          self.with_inner(|inner| {
>              let needs_drop = core::mem::needs_drop::<T::JobDataType>();
>              let mut jq_locked = inner.lock();
>              let mut jq = jq_locked.as_mut().project();
>  
>              if seqno < *jq.last_completed {
>                  return;
>              }
>  
>              while let Some(mut job) = jq.running_jobs.pop_front() {
>                  if job
>                      .as_mut()
>                      .project()
>                      .fence
>                      .as_mut()
>                      .expect("fence not present")
>                      .as_fence()
>                      .seqno()
>                      > seqno
>                  {
>                      jq.running_jobs.push_front(job);
>                      break;
>                  }
>  
>                  *jq.capacity += u64::from(job.cost);
>  
>                  let fence = job.fence.as_ref().expect("fence not present");
>                  fence.signal(Ok(()));
>  
>                  if needs_drop {
>                      jq.drop_list.push_back(job);
>                  }
>                  // Otherwise, job is dropped here directly. Its fence won't perform
>                  // an RCU grace period wait.
>              }

[Severity: High]
Can calling fence.signal() and dropping job while holding inner.lock()
(SpinLockIrq) in JobQueue::complete_jobs_up_to_seqno() (as well as in
SubmitWorker::run() and JobQueue::teardown()) deadlock?

DriverFence::signal() acquires the fence spinlock and synchronously invokes
all registered FenceCallback::on_signal() callbacks while inner.lock() is
held. If a callback on that fence submits a job via Job::submit() or calls
complete_jobs_up_to_seqno() on the same JobQueue, it will attempt to acquire
inner.lock() on the same CPU and deadlock.

Furthermore, when !needs_drop, dropping job at the end of the loop while
holding inner.lock() drops job.dependencies, which calls
FenceCallbackRegistration::drop() -> bindings::dma_fence_remove_callback()
and acquires each dependency fence's spinlock under inner.lock() (lock order
inner.lock() -> fence->lock). Meanwhile, DependencyWaker::on_signal() runs
under fence->lock, and any fence callback that takes inner.lock() establishes
the opposite lock order (fence->lock -> inner.lock()), creating an AB-BA
deadlock.

[ ... ]
> @@ -785,42 +785,42 @@ impl<T: JobQueueOps + Send + Sync> JobQueue<T> {
>      pub fn teardown<E>(self, error: Error)
>      where
>          Error: From<E>,
>      {
>          self.with_inner(|inner| {
>              let mut jq = inner.lock();
>  
>              // We don't drop the jobs and fences here while we hold the lock. Just
>              // signal with the error. The dropping lists will then drop them locklessly.
>  
>              while let Some(job) = jq.running_jobs.pop_front() {
>                  let fence = job.fence.as_ref().expect("fence not present");
>                  fence.signal(Err(error));
>  
>                  jq.drop_list.push_back(job);
>              }
>          })
>      }
>  }

[Severity: Medium]
As reported by Sashiko on RFC v6 [3], how can a caller invoke
JobQueue::teardown() when JobQueue<T> is #[pin_data(PinnedDrop)] (!Unpin) and
teardown<E>(self, error: Error) takes self by value with an unconstrained
type parameter E?

Because JobQueue<T> is pin-initialized (for example via KBox::pin_init), it
cannot be moved out of a Pin pointer by value, and E cannot be inferred from
the arguments since error has type Error rather than E. In addition,
teardown() only drains and signals jq.running_jobs while leaving
jq.waiting_jobs untouched.

>  
>  #[pinned_drop]
>  impl<T: JobQueueOps + Send + Sync> PinnedDrop for JobQueue<T> {
>      fn drop(self: Pin<&mut Self>) {
>          // Revoke to prevent still existing DependencyWakers from kicking the
>          // SubmitWorker again before we could deregister them.
>          self.revoked.store(true, Relaxed);
>          self.srcu.synchronize();
>          self.with_inner(|inner| {
>              let mut inner = inner.lock();
>              let waiting_jobs = core::mem::take(&mut inner.waiting_jobs);
>              let running_jobs = core::mem::take(&mut inner.running_jobs);
>              let drop_list = core::mem::take(&mut inner.drop_list);
>              drop(inner);
>              drop(waiting_jobs);
>              drop(running_jobs);
>              drop(drop_list);
>          })
>      }
>  }

[Severity: High]
As reported by Sashiko on RFC v6 [4], when JobQueue::teardown() or
<JobQueue<T> as PinnedDrop>::drop() runs, aren't jobs in waiting_jobs (and
running_jobs on normal drop) dropped without signaling their fences first?

JobQueue::teardown() does not set self.revoked or signal fences in
jq.waiting_jobs with Err(error), and <JobQueue<T> as PinnedDrop>::drop()
drops waiting_jobs and running_jobs directly without signaling their
DriverFences with ECANCELED. As a result, dropping those jobs hits the
unsignaled fallback in DriverFence::drop(), which logs an error ("DriverFence
drops unsignaled. Danger of memory corruption!") and signals waiting_jobs with
ECANCELED instead of the error passed to teardown().

[1] https://lore.kernel.org/r/20261016085724.1031766-7-phasta@kernel.org
[2] https://lore.kernel.org/r/20261016085724.1031766-7-phasta@kernel.org
[3] https://lore.kernel.org/r/20261016085724.1031766-7-phasta@kernel.org
[4] https://lore.kernel.org/r/20261016085724.1031766-7-phasta@kernel.org

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

Re: [RFC PATCH v7 8/8] samples: rust: Add JobQueue Tester

[Younes Akhouayri]

On Tue, 3 Nov 2026 16:31:38 +0100, Philipp Stanner wrote:
> +            drop(jq); // Release lock.
> +            T::run_job(work.fctx, runnable_job);
> +            jq = work.inner.lock(); // Retake lock.
> +
> +            // run_job() above needs to be called without lock protection. Thus,
> +            // it is possible that the driver instructed completion right before
> +            // we could re-take the lock.
> +            if jq.last_completed >= seqno {
> +                fence.signal(Ok(()));
> +
> +                // Drop here directly if possible. Otherwise, let the DropWorker
> +                // take care of it.
> +                if core::mem::needs_drop::<T::JobDataType>() {
> +                    jq.drop_list.push_back(job);
> +                }
> +            } else {
> +                jq.running_jobs.push_back(job);
> +            }

Hi Philip,

The job's cost is subtracted from jq.capacity further up in the loop.
complete_jobs_up_to_seqno() restores capacity only for jobs it pops from
running_jobs, and a job that takes this branch never gets there. So each
completion that wins the race against the worker leaves `cost` credits
consumed for good, and a queue with a small capacity would stall after a few
of them.

The tester takes this branch too, since it calls complete_jobs_up_to_seqno()
right after submit, before the worker has run. Each job costs 1 out of a
capacity of 1_000_000, so the leak never shows there.

Is there a reason the credits are meant to stay consumed here, or should
this branch do

+               *jq.as_mut().project().capacity += u64::from(job.cost);

as well?

> +                last_completed: 0,

FenceContext::new(initial_seqno, ...) hands out initial_seqno as the first
fence seqno, and both the doc example and the tester pass 0. For that first
job the check above becomes `0 >= 0`, so the worker signals the fence Ok
right after run_job() returns, while the hardware is still working on it.

Should last_completed start at initial_seqno.wrapping_sub(1), or is
initial_seqno >= 1 an intended requirement that just needs documenting?

Best regards,
Younes

Re: [RFC PATCH v7 8/8] samples: rust: Add JobQueue Tester

[Philipp Stanner]

On Tue, 2026-11-03 at 22:27 +0000, Younes Akhouayri wrote:
> On Tue, 3 Nov 2026 16:31:38 +0100, Philipp Stanner wrote:
> > +            drop(jq); // Release lock.
> > +            T::run_job(work.fctx, runnable_job);
> > +            jq = work.inner.lock(); // Retake lock.
> > +
> > +            // run_job() above needs to be called without lock protection. Thus,
> > +            // it is possible that the driver instructed completion right before
> > +            // we could re-take the lock.
> > +            if jq.last_completed >= seqno {
> > +                fence.signal(Ok(()));
> > +
> > +                // Drop here directly if possible. Otherwise, let the DropWorker
> > +                // take care of it.
> > +                if core::mem::needs_drop::<T::JobDataType>() {
> > +                    jq.drop_list.push_back(job);
> > +                }
> > +            } else {
> > +                jq.running_jobs.push_back(job);
> > +            }
> 
> Hi Philip,

Hello,

> 
> Is there a reason the credits are meant to stay consumed here, or should
> this branch do
> 
> +               *jq.as_mut().project().capacity += u64::from(job.cost);
> 
> as well?

Nope, that's simply a bug. Well seen. This RFC has only seen basic
testing so far, because we still have to solve some quite ground laying
design questions (see cover-letter). Also the credit-count system was
re-added relatively recently.

Nevertheless, fixed this one locally, thanks for pointing it out

> 
> > +                last_completed: 0,
> 
> FenceContext::new(initial_seqno, ...) hands out initial_seqno as the first
> fence seqno, and both the doc example and the tester pass 0. For that first
> job the check above becomes `0 >= 0`, so the worker signals the fence Ok
> right after run_job() returns, while the hardware is still working on it.
> 
> Should last_completed start at initial_seqno.wrapping_sub(1), or is
> initial_seqno >= 1 an intended requirement that just needs documenting?

Typically the drivers will want to pass 1 as initial sequence number,
because they have the same problem you're describing, just on the
hardware: it's initialized to 0, so the first job would be successful
immediately. So they wanna start at 1.

Not sure how we should best address this.

@Daniel, maybe we can just remove `initial_seqno` and always start at
1? I'm not sure.

Thx for the review,

P.