(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?
> +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.
> +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"?
> + //
> + // 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?
> +
> + 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.
> + }
> +}
> +
> +// 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.
