Re: [PATCH 3/4] rust: Add dma_fence abstractions

[Philipp Stanner <phasta@mailbox.org>]

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.