On Sat, 2026-05-30 at 17:16 +0200, Danilo Krummrich wrote:
(Not a full review, but a few drive-by comments.)
On Sat May 30, 2026 at 4:35 PM CEST, Philipp Stanner wrote:
+#[allow(unused_unsafe)]
What is this needed for?
You know that :-P
+impl<F: Send + Sync + DriverFenceAllowedData, C: Send + Sync> FenceCtx<F, C> {
<snip>
+impl<F: Send + Sync, C: Send + Sync> PinnedDrop for FenceCtx<F, C> { + fn drop(self: Pin<&mut Self>) { + // SAFETY: `rcu_barrier()` is always safe to be called. + unsafe { bindings::rcu_barrier() };
We should probably add a safe function for this.
ACK.
+impl<T: FenceCb> FenceCbRegistration<T> { + /// Register a callback on a fence. + /// + /// On success the callback is pinned in place and will fire when the fence + /// signals. On `AlreadySignaled` the callback is returned to the caller so + /// that owned resources can be reclaimed. + pub fn new<'a>(fence: &'a Fence, callback: T) -> impl PinInit<Self, CallbackError<T>> + 'a + where + T: 'a, + { + // Uses `pin_init_from_closure` instead of `try_pin_init!` so that on + // `-ENOENT` (already signaled) the callback can be read back from the + // partially-initialized slot and returned through the error.
Seems a bit odd that this needs pin_init_from_closure(). You can still use try_pin_init!() with &this in Self an a _: initializer at the end in the worst case. But the fence and callback fields should be fine to initialize "normally"?
I'll investigate that.
+ // + // SAFETY: `pin_init_from_closure` requires: + // - On `Ok(())`: the slot is fully initialized and valid for `Drop`. + // - On `Err(_)`: the slot is clean, i.e.: no partially-initialized fields + // remain, and the slot can be deallocated without dropping. + // + // We uphold this as follows: + // - On success: all three fields are initialized. Ok(()) is returned. + // - On ENOENT (already signaled): `callback` and `fence` are read back + // from the slot via `ptr::read`, leaving the slot clean. `cb` was + // initialized by `dma_fence_add_callback` (it calls + // `INIT_LIST_HEAD(&cb->node)` even on error), but `cb` is + // `Opaque<dma_fence_cb>` which has no `Drop`, so not dropping it is + // fine. The callback is returned through `AlreadySignaled(T)`. + // - On other errors: same cleanup as ENOENT, error returned as + // `Other(e)`. + unsafe { + pin_init_from_closure(move |slot: *mut Self| { + let slot_callback = &raw mut (*slot).callback; + let slot_fence = &raw mut (*slot).fence; + let slot_cb = &raw mut (*slot).cb;
+ // Write callback and fence first — must be visible before + // dma_fence_add_callback makes the registration live. + core::ptr::write(slot_callback, callback); + core::ptr::write(slot_fence, ARef::from(fence));
+ let ret = to_result(bindings::dma_fence_add_callback( + fence.inner.get(), + Opaque::cast_into(slot_cb), + Some(Self::dma_fence_callback), + ));
+ match ret { + Ok(()) => Ok(()), + Err(e) => { + // Read back what we wrote to leave the slot clean. + let cb_back = core::ptr::read(slot_callback); + let _fence_back = core::ptr::read(slot_fence);
What's the purpose of _fence_back?
Relic. Will rework.
+ if e.to_errno() == ENOENT.to_errno() { + Err(CallbackError::AlreadySignaled(cb_back)) + } else { + Err(CallbackError::Other(e)) + } + } + } + }) + } + } + /// Signal the fence. This will invoke all registered callbacks. + pub fn signal(self, res: Result) { + let fence = self.as_raw(); + let mut fence_flags: usize = 0; + let flag_ptr = &raw mut fence_flags;
+ // 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 the `DriverFence` has been dropped. + unsafe { + bindings::dma_fence_lock_irqsave(fence, flag_ptr); + if !bindings::dma_fence_is_signaled_locked(fence) { + if let Err(err) = res { + bindings::dma_fence_set_error(fence, err.to_errno()); + } + bindings::dma_fence_signal_locked(fence); + } + bindings::dma_fence_unlock_irqrestore(fence, flag_ptr); + }
Please use a single unsafe block per unsafe function call, here and in a few other places.
Is that an official rule? If so, the linters should inform about it.
At first glance, I don't see any advantage to it and the disadvantage of greatly reducing readability.
+ } +}
+// SAFETY: Fences are literally designed to be shared between threads. +unsafe impl<F: Send + Sync, C: Send + Sync> Send for DriverFence<F, C> {}
+impl<F: Send + Sync, C: Send + Sync> Deref for DriverFence<F, C> { + type Target = F;
+ fn deref(&self) -> &Self::Target { + // SAFETY: Thanks to refcounting, `data` is always valid as long as `self` is. + let data = unsafe { &*self.data.as_ptr() };
+ &data.data + } +}
+/// A borrowed [`DriverFence`]. All you can do with it is access your user data +/// and obtain a [`Fence`]. +pub struct DriverFenceBorrow<F: Send + Sync, C: Send + Sync> {
This misses the lifetime bound, which is the purpose of this struct.
+ /// The actual content of the fence. Lives in a raw pointer so that its + /// memory can be managed independently. Valid until both the [`DriverFence`] + /// and all associated [`Fence`]s have disappeared. + data: NonNull<DriverFenceData<F, C>>,
Why not use ManuallyDrop<DriverFence>? This way you would only need a Deref impl to &'a DriverFence.
This way you basically reimplement the DriverFence type just without the destructor.
Good idea, will do.
P.