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

[Alice Ryhl <aliceryhl@google.com>]

On Mon, Jun 01, 2026 at 02:26:17PM +0200, Philipp Stanner wrote:
> On Mon, 2026-06-01 at 10:36 +0000, Alice Ryhl wrote:
> > On Sat, May 30, 2026 at 04:35:11PM +0200, Philipp Stanner wrote:
> > > +        unsafe {
> > > +            bindings::dma_fence_remove_callback(self.fence.as_raw(), self.cb.get());
> > > +        }
> > 
> > Formatting nit: Usually the ; goes outside the unsafe block.
> 
> I could have sworn that it was rustfmt who did that? Maybe because the
> ; was inside to begin with.

Indeed, rustfmt will not change whether the ; is inside or outside the
unsafe block.

> > > +/// A trait to enforce that all data in a [`DriverFence`] either does not need
> > > +/// drop, or lives in a [`RcuBox`].
> > > +pub trait DriverFenceAllowedData: private::Sealed {}
> > > +
> > > +mod private {
> > > +    pub trait Sealed {}
> > > +}
> > > +
> > > +impl<F: Copy> DriverFenceAllowedData for F {}
> > > +impl<F: Send> DriverFenceAllowedData for RcuBox<F> {}
> > > +
> > > +impl<F: Copy> private::Sealed for F {}
> > > +impl<F: Send> private::Sealed for RcuBox<F> {}
> > 
> > Why sealed? Just make the trait unsafe and require the things you
> > require from the user.
> 
> This is far better. We definitely only allow the user to pass A or B,
> and only then it compiles.

What if I have another type that I want to use here? For example, maybe
I have a struct containing a copy field and an RcuBox. Or maybe I have
an ARef<_> of some C type that uses rcu for cleanup. Then I must edit
this file to add support for it?

> The unsafe implementation could be messed up.
> 
> I thought that's what Sealed is for. Or isn't it?

Sealed is for making 100% sure that downstream crates/drivers cannot
provide their own implementations. But I don't see why you need that.
All you require is that the value remains valid for one grace period
after cleanup begins. As long as the type satisfies that, you are happy.
An unsafe trait can require that sort of requirement from the user.

I think what you want is expressed well by `RcuFreeSafe` from this
thread:
https://rust-for-linux.zulipchat.com/#narrow/channel/291566-Library/topic/Consolidate.20.60PollCondVarBox.60.20into.20.60Rcu.2ABox.60/near/598726724

> > > +/// A synchronization primitive mainly for GPU drivers.
> > > +///
> > > +/// Fences are always reference counted. The typical use case is that one side registers
> > > +/// callbacks on the fence which will perform a certain action (such as queueing work) once the
> > > +/// other side signals the fence.
> > > +///
> > > +/// # Examples
> > > +///
> > > +/// ```
> > > +/// use kernel::dma_buf::{DriverFence, FenceCtx, FenceCb, FenceCbRegistration};
> > > +/// use kernel::str::CString;
> > > +/// use kernel::sync::{
> > > +///     aref::ARef,
> > > +///     rcu::RcuBox, //
> > > +/// };
> > > +/// use core::ops::Deref;
> > > +/// use core::fmt::Display;
> > 
> > Use fmt traits from kernel instead. (Actually, I don't think you use
> > Display at all here?)
> 
> I tried, see a few lines below:
> 
> > 
> > > +/// struct CallbackData { }
> > > +///
> > > +/// impl FenceCb for CallbackData {
> > > +///     fn called(&mut self) {
> > > +///         pr_info!("DmaFence callback executed.\n");
> > > +///     }
> > > +/// }
> > > +///
> > > +/// let driver_name = CString::try_from_fmt(fmt!("dummy_driver"))?;
> > > +/// let timeline_name = CString::try_from_fmt(fmt!("dummy_timeline"))?;
> > > +///
> > > +/// let fctx = FenceCtx::new(driver_name, timeline_name, ())?;
> > > +///
> > > +/// let fence_data = CString::try_from_fmt(fmt!("dummy_data"))?;
> > > +/// // DriverFence::data must either not need drop, or live in an RcuBox.
> > > +/// let fence_data = RcuBox::new(fence_data, GFP_KERNEL)?;
> > > +///
> > > +/// let fence_alloc = fctx.as_arc_borrow().new_fence_allocation(fence_data)?;
> > > +/// let mut fence = fctx.new_fence(fence_alloc);
> > > +///
> > > +/// let cb_data = CallbackData { };
> > > +/// let waiting_fence = ARef::from(fence.as_fence());
> > > +/// let cb_reg = FenceCbRegistration::new(&waiting_fence, cb_data);
> > > +/// let cb_reg = KBox::pin_init(cb_reg, GFP_KERNEL)?;
> > > +///
> > > +/// // DriverFence implements Deref.
> > > +/// // FIXME: unit test claims that CString does not implement Display. Why?
> > > +/// // pr_info!("Fence's inner data is: {}", fence.deref().deref());
> 
> Lazily, I was hoping that someone here will tell me how that is
> supposed to be done correctly 8-)

This specific code could be written cleaner as &**fence, but it looks
like CString should implement fmt::Display like CStr does.

> > > +// SAFETY: The Rust dma_fence abstractions are already designed around the inner
> > > +// C `dma_fence`, which can serve safely as the identification point when being
> > > +// owned by C. Moreover, safety is ensured by not dropping `DriverFence` and by
> > > +// only allowing operations without side effects on the Borrowed type.
> > > +unsafe impl<F: Send + Sync + 'static, C: Send + Sync + 'static> ForeignOwnable
> > > +    for DriverFence<F, C>
> > > +{
> > > +    // `DriverFence` is merely a wrapper around a raw pointer. Thus, we can just
> > > +    // use it directly.
> > > +    type Borrowed<'a> = DriverFenceBorrow<F, C>;
> > > +    type BorrowedMut<'a> = DriverFenceBorrow<F, C>;
> > > +
> > > +    const FOREIGN_ALIGN: usize = core::mem::align_of::<bindings::dma_fence>();
> > > +
> > > +    fn into_foreign(self) -> *mut c_void {
> > > +        let fence = self;
> > > +
> > > +        let ptr = fence.as_raw();
> > > +
> > > +        // DriverFence must not drop.
> > > +        core::mem::forget(fence);
> > 
> > Nit: Modern Rust uses ManuallyDrop instead of forget().
> 
> You mean still take `self` here, then stuff it into ManuallyDrop and
> let it go out of scope, aye?

I mean this:

let fence = ManuallyDrop::new(self);
let ptr = fence.as_raw();

This avoids moving `fence` after calling `as_raw()`.

Alice