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.
