Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider: - [Critical] drm/job_queue: recursive spinlock deadlock when signaling intra-queue dependencies - [High] drm/job_queue: job is permanently lost when queue capacity is exceeded - [High] drm/job_queue: submit_worker completely ignores dependencies and runs unready jobs - [High] drm/job_queue: synchronous job completion is permanently missed during run_job - [High] drm: JobQueue runs run_job in atomic context due to RevocableGuard - [Medium] drm/job_queue: capacity is leaked when SubmitWorker cleans up signaled jobs - [Medium] drm/job_queue: JobQueue API is unusable due to borrow checker conflict --
commit 4ab6c27823ae4ac383ac1e5112edd966e2143dbd Author: Philipp Stanner <[email protected]> rust: drm: Add JobQueue JobQueue is a pure Rust component that handles job submissions for GPUs with firmware scheduling, i.e., GPUs that support one exclusive ring per execution context. [ ... ] > diff --git a/rust/kernel/drm/job_queue.rs b/rust/kernel/drm/job_queue.rs > new file mode 100644 > index 0000000000000..b5c7f4ecb3f46 > --- /dev/null > +++ b/rust/kernel/drm/job_queue.rs > @@ -0,0 +1,525 @@ > +// SPDX-License-Identifier: GPL-2.0 [ ... ] > +impl<T: JobQueueOps + Send + Sync> ScopedWorkItem for SubmitWorker<'_, T> { > + fn run(work: &ScopedWorkRef<Self>) { > + let Some(jq_accessor) = work.jq.inner.try_access() else { > + return; > + }; [Severity: High] The JobQueue documentation explicitly states that jobs are submitted in process context, which normally permits sleeping. However, does this try_access() call enter an atomic context? When try_access() returns a RevocableGuard, it intrinsically holds rcu::read_lock(). Because the jq_accessor guard remains alive across the subsequent call to T::run_job(), the driver callback is executed entirely within an RCU read-side critical section, where sleeping is forbidden. Will this regression cause a "scheduling while atomic" panic if a driver relies on the documented process context guarantee to sleep, wait on mutexes, or use GFP_KERNEL allocations inside run_job? > + > + let mut jq = jq_accessor.lock(); > + > + while let Some(mut job) = > jq.as_mut().project().waiting_jobs.pop_front() { > + let new_capacity = jq.capacity as i64 - job.cost as i64; [Severity: High] Does this bypass the dependency system? JobQueue::submit() pushes jobs unconditionally to waiting_jobs, and this loop iterates over them and executes them as long as there is sufficient queue capacity. However, it never checks if job.nr_of_deps == 0 before executing the job. Will this regression result in jobs being sent to the hardware immediately, regardless of pending dependency fences? > + if new_capacity < 0 { > + break; > + } [Severity: High] When this loop encounters a job that exceeds the available capacity, it executes a break to exit. Because the job was already removed from the waiting_jobs list via pop_front(), breaking the loop drops the local job variable, destroying it. Since the job is never pushed back onto waiting_jobs, its hardware operations will be lost and its completion fence will never signal. Will this regression cause a permanent hang for anything depending on it? > + *jq.as_mut().project().capacity = new_capacity as u64; > + > + let runnable_job = JobRunnable { > + seqno: job.fence().as_ref().expect("fence not > present").as_fence().seqno(), > + data: job.fence().as_ref().expect("fence not > present").deref(), > + }; > + > + let jq_data = work.jq.fctx.deref().deref(); > + > + drop(jq); > + T::run_job(jq_data, runnable_job); > + // The driver could now have taken the lock here and try to > complete > + // this very job. It would then not get signaled because it was > not > + // yet in the running list. However, since the driver completes > jobs > + // by sequence number, its subsequent calls to > jq.complete_jobs_up_to_seqno() > + // will signal this job, because then it will be in the > running_jobs > + // list. > + // We accept this race because it makes the implementation much > + // simpler and avoids locking issues. Otherwise, > complete_jobs_up_to_seqno() > + // would need a distinct signaler-object-list to avoid races > between > + // this work item and said function. > + jq = jq_accessor.lock(); > + > + jq.as_mut().project().running_jobs.push_back(job); [Severity: High] The code drops the queue lock before calling T::run_job(). If the driver completes the job immediately (e.g. via synchronous execution or a very fast hardware interrupt) and calls complete_jobs_up_to_seqno(), the completion handler will search running_jobs. Since the worker hasn't yet pushed the job to running_jobs here, the handler will completely miss it. If this is the last job in a burst, and no subsequent jobs trigger completion, will this regression leave the job hanging forever in running_jobs without its fence signaling? > + } > + > + // Now clean up all already-signaled jobs, dropping their fences. > + let mut jq = jq.as_mut().project(); > + > + let mut cursor = jq.running_jobs.cursor_front(); > + > + while let Some(job) = cursor.peek_next() { > + let mut job = job.remove(); > + > + if !job.fence().as_mut().expect("fence not > present").as_fence().is_signaled() { > + jq.running_jobs.push_front(job); > + // Jobs complete in order. No point in checking the rest. > + break; > + } [Severity: Medium] This cleanup loop iterates over running_jobs and removes any job whose fence is signaled. Unlike the standard completion path in complete_jobs_up_to_seqno() which restores capacity via *jq.capacity += job.cost, this loop drops the job without updating jq.capacity. Will this regression cause a permanent reduction in the queue's tracked capacity? [ ... ] > + /// Create a new job. > + pub fn new_job(&'a self, cost: u32, data: T::JobDataType) -> > Result<Job<'a, T>> { [Severity: Medium] This borrows the JobQueue immutably (&'a self) for the lifetime of the returned Job. Later, JobQueue::submit takes a mutable reference (&mut self) and the Job. Because self cannot be borrowed mutably while the job's immutable borrow is active, this creates a strict compile error. While the documentation comment acknowledges this problem, does this regression make the API structurally impossible to call from driver code? [ ... ] > + pub fn complete_jobs_up_to_seqno(&self, seqno: u64, status: Result) { > + // unwrap() can't fire because `self` is valid. > + let jq = self.inner.try_access().unwrap(); > + let mut jq_locked = jq.lock(); > + let mut jq = jq_locked.as_mut().project(); > + > + let mut cursor = jq.running_jobs.cursor_front(); > + > + while let Some(job) = cursor.peek_next() { > + let mut job = job.remove(); > + > + if job.fence().as_mut().expect("fence not > present").as_fence().seqno() > seqno { > + jq.running_jobs.push_front(job); > + break; > + } > + > + *jq.capacity += job.cost as u64; > + > + if let Some(fence) = job.fence() { > + fence.signal(status); [Severity: Critical] This function acquires the queue's SpinLockIrq (jq.lock()) and calls fence.signal() on completed jobs. If another job in the same queue was configured to depend on this fence, DependencyWaker::on_signal() is executed synchronously by the dma-fence subsystem. Since on_signal() then attempts to acquire the exact same queue spinlock via jq.deref().lock(), will this regression result in a guaranteed recursive spinlock deadlock on the same CPU? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
