Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Calling Fence::is_signaled() from within a fence callback for the same fence will cause a deadlock. - [High] FenceContext::drop unconditionally calls rcu_barrier(), which can lead to sleeping in atomic context panics and severe performance issues. --
--- Patch [4]: [PATCH v9 4/5] rust: Add dma_fence abstractions --- commit 38c478417a422309c2a228acac8f860a0f96dd34 Author: Philipp Stanner <[email protected]> rust: Add dma_fence abstractions This commit introduces abstractions for C's dma_fence to be used by Rust GPU drivers. It implements DriverFence and Fence types to handle signaling, sequence numbering, and callback registration. > diff --git a/rust/kernel/dma_buf/dma_fence.rs > b/rust/kernel/dma_buf/dma_fence.rs > new file mode 100644 > index 0000000000000..e61b4b2d8b8cc > --- /dev/null > +++ b/rust/kernel/dma_buf/dma_fence.rs [ ... ] > +#[pinned_drop] > +impl<T: FenceContextOps + Send + Sync> PinnedDrop for FenceContext<T> { > + fn drop(self: Pin<&mut Self>) { > + // TODO: > + // It would be better if the fence context signals all forgotten > fences > + // itself. To do so, it would keep a list of unsignaled fences. That > + // list members would have to be pre-allocated (see > + // FenceCallback::new_fence_allocation()). > + if self.nr_of_unsignaled_fences.load(Relaxed) != 0 { > + panic!("Forgotten fences in FenceContext."); > + } > + > + rcu_barrier(); [Severity: High] Can dropping a FenceContext in atomic context cause a panic here? Because Rust drops objects implicitly when they go out of scope, a FenceContext dropped in an atomic context (such as inside an interrupt handler or under a spinlock) will trigger a 'sleeping in atomic context' panic since rcu_barrier() blocks system-wide. Even if dropped safely in process context, could waiting on a system-wide RCU barrier during the destruction of every FenceContext cause severe performance bottlenecks? [ ... ] > + pub fn is_signaled(&self) -> bool { [ ... ] > + // 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 > + } > + > + fn lock(&self) -> FenceGuard { > + let mut guard = FenceGuard { > + inner: self.as_raw(), > + flags: 0, > + }; > + > + // SAFETY: fence is valid because self is valid. flag_ptr is > + // merely a pointer to an integer, whose lifetime is tied to the > guard > + // object. > + unsafe { bindings::dma_fence_lock_irqsave(guard.inner, &raw mut > guard.flags) }; [Severity: High] Could this result in a deadlock if Fence::is_signaled() is called from within a fence callback? When a fence is signaled, the C backend (dma_fence_signal_locked) acquires the fence's spinlock and iterates over the registered callbacks, executing them synchronously while holding the lock. If a Rust FenceCallback implementation calls is_signaled() on that same fence, it invokes self.lock() which will attempt to acquire the exact same spinlock via dma_fence_lock_irqsave(), leading to a non-recursive spinlock deadlock. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
