Re: [PATCH v9 4/5] rust: Add dma_fence abstractions

[Philipp Stanner <phasta@mailbox.org>]

On Thu, 2026-08-06 at 14:56 +0100, Gary Guo wrote:
> On Wed Aug 5, 2026 at 3:59 PM BST, Philipp Stanner wrote:
> > 

[…]

(feel free to crop huge mails like this)


> > +/// Error type for fence callback registration.
> > +///
> > +/// Generic over `T` so that `AlreadySignaled` can return the callback to the
> > +/// caller, allowing it to reclaim any resources owned by the callback (e.g.,
> > +/// a fence handle that needs to be signaled).
> > +#[derive(Debug)]
> > +pub enum CallbackError<T = ()> {
> 
> What is this `= ()` used for?

A relic, can remove.

> 
> > +    /// The fence was already signaled. The callback is returned so the caller
> > +    /// can extract owned resources without losing them.
> > +    AlreadySignaled(T),
> > +    /// Some other error occurred during registration.
> > +    Other(Error),
> > +}
> > +
> > +impl<T> From<CallbackError<T>> for Error {
> 
> #[inline]
> 
> > +    fn from(err: CallbackError<T>) -> Self {
> > +        match err {
> > +            CallbackError::AlreadySignaled(_) => ENOENT,
> > +            CallbackError::Other(e) => e,
> > +        }
> > +    }
> > +}
> > +
> > +impl<T> From<AllocError> for CallbackError<T> {
> 
> #[inline]
> 
> > +    fn from(e: AllocError) -> Self {
> > +        CallbackError::Other(Error::from(e))
> > +    }
> > +}
> > +
> > 

[…]

> > +pub trait FenceCallback: Send + 'static {
> > +    /// Called when the fence is signaled.
> > +    ///
> > +    /// This is called from the fence signaling path, which may be in interrupt
> > +    /// context or with locks held, which is why `self` is only borrowed, so that
> > +    /// it cannot drop. Implementations must not sleep or perform
> > +    /// long-running operations.
> > +    ///
> > +    /// An implementation likely wants to inform itself (e.g., through a work item)
> > +    /// within this callback that the associated [`FenceCallbackRegistration`]
> > +    /// can now be dropped.
> > +    fn called(&mut self);
> 
> The name feels a bit awkward to me. I think this should either look like an
> action on the callback, in which case "call" or describe an event on the fence,
> i.e. "on_signal" or "signaled". Naming it "called" is very weird because it's
> not a event that is triggered when something is "called".

on_signal() sounds good.

> 
> > +}
> > +
> > 

[…]

> > +#[pinned_drop]
> > +impl<T: FenceCallback> PinnedDrop for FenceCallbackRegistration<T> {
> > +    fn drop(self: Pin<&mut Self>) {
> > +        // Always call dma_fence_remove_callback, even if `callback` has already
> > +        // been taken by `dma_fence_callback`.  This is necessary for
> 
> Is this still up-to-date? You're not taking callback anymore in
> `dma_fence_callback`.

The comment is outdated, but the race prevention is still necessary.
I'll update the text.

> 
> > 

[…]

> > +impl Deref for FenceGuard {
> > +    type Target = *mut bindings::dma_fence;
> 
> Why not store and return `&Fence`?

Sounds good, although it would be cool if the FenceGuard directly
derefs to the raw pointer, since the need for accessing the latter with
the lock held is the reason the guard exists.

> 
> > +
> > 

[…]

> 
> #[inline] here and many more below.

I do not understand the Rust policy regarding inline at all. When do
you need it and when not?

> 
> > +    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) };
> > +
> > +        guard
> > +    }
> > +
> > +    /// Get the fence's sequence number.
> > +    pub fn seqno(&self) -> u64 {

inline?

> > +        // SAFETY: Valid because `self` is valid.
> > +        unsafe { (*self.as_raw()).seqno }
> > +    }
> > +
> > +    fn as_raw(&self) -> *mut bindings::dma_fence {
> > +        self.inner.get()
> > +    }
> > +
> > +    /// Create a [`Fence`] from a raw C [`bindings::dma_fence`].
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// `ptr` must point to an initialized fence that is embedded into a [`Fence`].
> > +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::dma_fence) -> &'a Self {

inline?

> > +        // SAFETY: Safe as per the function's overall safety requirements.
> > +        unsafe { &*ptr.cast() }
> > +    }
> > +}

In C, we spent literally decades to get it out of people's heads to use
the inline keyword, because "the compiler knows better than you". It
should only be used for code in headers (and I even think that there
should not be code in headers).

What is the Rust equivalent?


> > +
> > 

[…]

> > +        // SAFETY: `fence` is valid because `self` is valid. The lock must be
> > +        // held, which we acquired directly above.
> > +        if !unsafe { bindings::dma_fence_test_signaled_flag(*fence.deref()) } {
> 
> These `*fence.deref()` are quite weird as consequence of `FenceGuard` design.
> If `FenceGuard` just derefs to `&Fence` then this can be `fence.as_raw()`.

True; it's good to fix this.

> 
> > +            if let Err(err) = res {
> > +                // SAFETY: `fence` is valid because `self` is valid. The fence
> > +                // must not have been signaled yet, which we check directly above.
> > +                unsafe { bindings::dma_fence_set_error(*fence.deref(), err.to_errno()) };
> > +            }
> > +            // SAFETY: `fence` is valid because `self` is valid. The lock must
> > +            // be held, which we acquired above.
> > +            unsafe { bindings::dma_fence_signal_locked(*fence.deref()) };
> > +        }
> > +
> > +        // SAFETY: `self.data` is valid because `self` is valid.
> > +        let fctx = unsafe { self.data.as_ref().fctx };
> > +        let _ = fctx.nr_of_unsignaled_fences.fetch_sub(1, Relaxed);
> 
> Drop impl of `self` here will neededlessly take lock again before checking it's
> signaled already and unlock.

Yes, I know.
So? What could we do about it?

We need to take the lock when signaling, and we need to take it when
dropping to check if everyone was kind and did indeed signal before.

What you describe is a lock-relock on the same CPU, and it is unlikely
that there are contenders (only contender right now could be
Fence::is_signaled(), which is nothing upcoming users will poll
intensively).

tl;dr I don't see a problem with that

> 
> > +    }
> > +}
> > 

[…]

> > +        // SAFETY: `call_rcu()` is always safe to be called. `rcu_head_ptr` was
> > +        // created validly above. The module must perform a `synchronize_rcu()`
> > +        // or `rcu_barrier()` call to guard against module unload.
> > +        unsafe { bindings::call_rcu(rcu_head_ptr, Some(drop_driver_fence_data::<T>)) };
> 
> I thought at some point it was mentioned that we want a fast path
> 
>     if !mem::needs_drop::<...>() {
>     }

True. Added it.

> 
> ?
> 
> > +    }
> > +}
> > +


Regarding all your nits and comments I skipped above, I agree with them
for the most part and will address them in v10.


P.