From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f74.google.com (mail-wr1-f74.google.com [209.85.221.74]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6D5EE3019A4 for ; Mon, 9 Feb 2026 11:30:28 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.221.74 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1770636628; cv=none; b=tqyK86mL/tZPM4syHhJ7ilhZBMskOGdXrsZi76iOaMMNltnYLZUFVzn7OmBD4qHmW3h3hKkmlnZ8Ys2vRimY8JxoEPiUhNwWMwGY/ZvXoNURqETmDoU+omPyvCFpSqe8RRetha8de6yg7zReu1RMzF1JoghwKpL65u62Ka7B1RU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1770636628; c=relaxed/simple; bh=QoCkRBGUPdHeeEONxNSG/v7ypDaj+Noume7ntAehjWc=; h=Date:In-Reply-To:Mime-Version:References:Message-ID:Subject:From: To:Cc:Content-Type; b=kD5cObyZbP6ybhWMTM/5AX2CtOzvVEVb1sLSSNGn736r9rp0wqHHLIKAsJjqcoo7pcvt4t2SALZREfX/WIzwq4NXmh/oz15P2yrWb9J9yx5wDvc93T81sxaPxUUGF2+w4MFg8nvTypLztrd5BAJpMFzyeCbUy7QZmoFx/8v9LE8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=flex--aliceryhl.bounces.google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=QctJ8tL0; arc=none smtp.client-ip=209.85.221.74 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=flex--aliceryhl.bounces.google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="QctJ8tL0" Received: by mail-wr1-f74.google.com with SMTP id ffacd0b85a97d-4368060a5e5so1458731f8f.3 for ; Mon, 09 Feb 2026 03:30:28 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1770636627; x=1771241427; darn=vger.kernel.org; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=B6UWN0OsE7QW1x3g1701AwzacxmXzQP0I4E5tISve+w=; b=QctJ8tL0Ir22NjWvPOQSSTHg977cHg04QSPp8IRjrn+wEMI+QpaojmU0BDpM6LHddp z+Oqh6quBpSSKEBcd9161FZwB/3efrgGzaWMHEzcFjzwe2ACDN8LQrGcgfOlScCFUrwa nxUcFecZg0Di4tRl1to4UhK3KQmBFtzuM4T4RHrG2A/7E4Fve21RtYnts7IlNfly5aFk piQDQgcL6HigYd7sXqElBI8dXOpLl3E7PaaBoJ1nN+EJ2KuTSAF/J4nGW6LwBaKXmR7i jzsz9HSmvIYw981KSfVgCsJ+mWu7Z5X6tQFjNyQ3MQzLih6K+EA+iOf8YmbZet0jyXlM QU/A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1770636627; x=1771241427; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=B6UWN0OsE7QW1x3g1701AwzacxmXzQP0I4E5tISve+w=; b=tlYd8OWQ5jUm2xHcMp5UHCat5IDGT5Kg298b9q8OimrlNn0mKROY3N8xc9IJays3hM HBBgXNNNQimc/lKqdEda4IE4jyJtKJgspOrHUlfO8c8ZkHn0RbaH/JZkdIkJRfvpJ4Eh FSsZSQWA8Px2g8SFKBxoSt6PEIh2s1NqWnPk1U/bZhfGGCuDEFM3uZEaCU4YBdFrvOti KxXy7u1cZ5CmwiojlAvBKG5pV7jJgHI+R9Kr9A7eMWZBQv4xP4VyCz10UXfSd6eo66ul wOARS+HKEwL38igDXco2d1+1Va4Jx5lEYe1/IG+o9HqJM+3S4tqWLxiztiB1SsqYUR/j wy8Q== X-Forwarded-Encrypted: i=1; AJvYcCUcAvshC6YSURcYkSw/ytYSqYrkJ0W8KRdAORsRe7szt1PjYYbNlqMXnuVBqfNTac6N6AM2eC3GAtMxVGk=@vger.kernel.org X-Gm-Message-State: AOJu0YxUNqzc2UHtSxGuKSGhVRsP6//SAuTlXnxd03cCKulR3kyNroMj RzX0KoSS7D3w4MlpXCFaDPUQ9AX+pXK3f0DkUvDZqDWTLI6vpbmCJoWQvrcmWzB9/inUngUjXMx hUpy5WoH4LEx7SeHLqw== X-Received: from wrgb9.prod.google.com ([2002:a05:6000:3c9:b0:437:7057:b172]) (user=aliceryhl job=prod-delivery.src-stubby-dispatcher) by 2002:a05:6000:230a:b0:437:678b:83ca with SMTP id ffacd0b85a97d-437678b8589mr8686513f8f.50.1770636626863; Mon, 09 Feb 2026 03:30:26 -0800 (PST) Date: Mon, 9 Feb 2026 11:30:25 +0000 In-Reply-To: <20260203081403.68733-4-phasta@kernel.org> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 References: <20260203081403.68733-2-phasta@kernel.org> <20260203081403.68733-4-phasta@kernel.org> Message-ID: Subject: Re: [RFC PATCH 2/4] rust: sync: Add dma_fence abstractions From: Alice Ryhl To: Philipp Stanner Cc: David Airlie , Simona Vetter , Danilo Krummrich , Gary Guo , Benno Lossin , "Christian =?utf-8?B?S8O2bmln?=" , Boris Brezillon , Daniel Almeida , Joel Fernandes , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, rust-for-linux@vger.kernel.org Content-Type: text/plain; charset="utf-8" On Tue, Feb 03, 2026 at 09:14:01AM +0100, Philipp Stanner wrote: > +void rust_helper_dma_fence_get(struct dma_fence *f) > +{ > + dma_fence_get(f); > +} > + > +void rust_helper_dma_fence_put(struct dma_fence *f) > +{ > + dma_fence_put(f); > +} > + > +bool rust_helper_dma_fence_begin_signalling(void) > +{ > + return dma_fence_begin_signalling(); > +} > + > +void rust_helper_dma_fence_end_signalling(bool cookie) > +{ > + dma_fence_end_signalling(cookie); > +} > + > +bool rust_helper_dma_fence_is_signaled(struct dma_fence *f) > +{ > + return dma_fence_is_signaled(f); > +} These should use the __rust_helper #define. See: https://lore.kernel.org/r/20260105-define-rust-helper-v2-0-51da5f454a67@google.com > +void rust_helper_spin_lock_init(spinlock_t *lock) > +{ > + spin_lock_init(lock); > +} > [..] > +#[pin_data] > +pub struct DmaFenceCtx { > + /// An opaque spinlock. Only ever passed to the C backend, never used by Rust. > + #[pin] > + lock: Opaque, > [...] > +} > + > +impl DmaFenceCtx { > + /// Create a new `DmaFenceCtx`. > + pub fn new() -> Result> { > + let ctx = pin_init!(Self { > + // Feed in a non-Rust spinlock for now, since the Rust side never needs the lock. > + lock <- Opaque::ffi_init(|slot: *mut bindings::spinlock| { > + // SAFETY: `slot` is a valid pointer to an uninitialized `struct spinlock_t`. > + unsafe { bindings::spin_lock_init(slot) }; > + }), We already have a __spin_lock_init() helper used by our SpinLock type. Can we just use that one instead of adding a new one? But actually I think it's simpler to just use SpinLock<()> as the type here. We have (or should add) a method to get the `state` field from a SpinLock<()>, which gets you a raw spinlock_t you can pass to C code. > +use core::{ > [...] > + sync::atomic::{AtomicU64, Ordering}, > +}; This should use kernel::sync::atomic instead. > +use kernel::c_str; > [...] > + extern "C" fn get_driver_name(_ptr: *mut bindings::dma_fence) -> *const c_char { > + c_str!("DRIVER_NAME_UNUSED").as_char_ptr() > + } > + > + extern "C" fn get_timeline_name(_ptr: *mut bindings::dma_fence) -> *const c_char { > + c_str!("TIMELINE_NAME_UNUSED").as_char_ptr() > + } We have c-strings literals now: extern "C" fn get_driver_name(_ptr: *mut bindings::dma_fence) -> *const c_char { c"DRIVER_NAME_UNUSED".as_char_ptr() } extern "C" fn get_timeline_name(_ptr: *mut bindings::dma_fence) -> *const c_char { c"TIMELINE_NAME_UNUSED".as_char_ptr() } > +pub trait DmaFenceCbFunc { > + /// The callback function. `cb` is a container of the data which the driver passed in > + /// [`DmaFence::register_callback`]. > + fn callback(cb: Pin>>) > + where > + Self: Sized; > +} Just make Sized into a super-trait. pub trait DmaFenceCbFunc: Sized { /// The callback function. `cb` is a container of the data which the driver passed in /// [`DmaFence::register_callback`]. fn callback(cb: Pin>>); } Probably also include 'static next to Sized instead of specifying it on register_callback(). > +impl DmaFenceCb { > + unsafe extern "C" fn callback( > + _fence_ptr: *mut bindings::dma_fence, > + cb_ptr: *mut bindings::dma_fence_cb, > + ) { > [...] > + // SAFETY: `cp_ptr` is the heap memory of a Pin> because it was created by > + // invoking ForeignOwnable::into_foreign() on such an instance. > + let cb = unsafe { > as ForeignOwnable>::from_foreign(cb_ptr) }; > + } > +} > [...] > + pub fn register_callback(&self, data: impl PinInit) -> Result { > + let cb = DmaFenceCb::new(data)?; > + let ptr = cb.into_foreign() as *mut DmaFenceCb; The ForeignOwnable trait provides no guarantees about where the void pointer points. The only legal usage of such a void pointer is to pass it to from_foreign() and borrow() and other similar methods defined on the ForeignOwnable trait. Casting it to DmaFenceCb and dereferencing it is illegal because it might point elsewhere in the box than at the DmaFenceCb value. (Yes for Box it happens to point there, but for e.g. Arc it points at the refcount_t value instead.) Please replace this usage of ForeignOwnable with Box::into_raw() / Box::from_raw() calls, or use ForeignOwnable::borrow[_mut]() to access the value. > + pub fn register_callback(&self, data: impl PinInit) -> Result { > + let cb = DmaFenceCb::new(data)?; > + let ptr = cb.into_foreign() as *mut DmaFenceCb; > + // SAFETY: `ptr` was created validly directly above. > + let inner_cb = unsafe { (*ptr).inner.get() }; > + > + // SAFETY: `self.as_raw()` is valid because `self` is refcounted, `inner_cb` was created > + // validly above and was turned into a ForeignOwnable, so it won't be dropped. `callback` > + // has static life time. > + let ret = unsafe { > + bindings::dma_fence_add_callback( > + self.as_raw(), > + inner_cb, > + Some(DmaFenceCb::::callback), > + ) > + }; > + if ret != 0 { > + return Err(Error::from_errno(ret)); > + } > + Ok(()) On error, this function leaks the DmaFenceCb allocation. It should be converted back to a Box so that the destructor may run. drop(unsafe { Box::from_raw(ptr) }); // or perhaps: drop(unsafe { DmaFenceCb::from_raw(...) }); Also this should use to_result(). > +impl DmaFenceCb { > + fn new(data: impl PinInit) -> Result>> { > [...] > + KBox::pin_init(cb, GFP_KERNEL) > [...] > +impl DmaFenceCtx { > + /// Create a new `DmaFenceCtx`. > + pub fn new() -> Result> { > [...] > + Arc::pin_init(ctx, GFP_KERNEL) Shouldn't the gfp flags be provided by the caller instead of hard-coding GFP_KERNEL here? > +unsafe impl AlwaysRefCounted for DmaFence { > + /// # Safety > + /// > + /// `ptr`must be a valid pointer to a [`DmaFence`]. > + unsafe fn dec_ref(ptr: NonNull) { > + // SAFETY: `ptr` is never a NULL pointer; and when `dec_ref()` is called > + // the fence is by definition still valid. > + let fence = unsafe { (*ptr.as_ptr()).inner.get() }; > + > + // SAFETY: Valid because `fence` was created validly above. > + unsafe { bindings::dma_fence_put(fence) } The safety requirements of `dec_ref()` as described here are incomplete. The caller must also give up ownership of one refcount to the value before they may call this method. But you may simply delete that section because this is a trait implementation, and the safety requirements are inherited from the declaration of AlwaysRefCounted. The safety comment on `let fence` is also not quite right. I don't think it's useful to talk about NULL because you require something stronger than NULL for this operation - for example `0xDEADBEEF as *mut DmaFence` is not NULL but would also be illegal here. A better wording would be to say that by the safety requirements, the caller passes a valid pointer to a `DmaFence`. And the safety comment on dma_fence_put() is also incomplete. The caller must pass ownership of one refcount, so the safety comment should mention why we can pass ownership of a refcount here (it is because caller must pass ownership of a refcount to us by the safety requirments). > + /// Mark the beginning of a DmaFence signalling critical section. Should be called once a fence > + /// gets published. > + /// > + /// The signalling critical section is marked as finished automatically once the fence signals. > + pub fn begin_signalling(&mut self) { I doubt it's legal to have a `&mut DmaFence` because I could call `mem::swap()` with two of them, which likely invalidates stuff inside `inner`. > + const fn ops_create() -> bindings::dma_fence_ops { > + // SAFETY: Zeroing out memory on the stack is always safe. > + let mut ops: bindings::dma_fence_ops = unsafe { core::mem::zeroed() }; No it's not always safe. If I have a local variable of type reference, then it's not safe to zero that value because NULL is not a legal value for references. The reason this is ok is because all fields of dma_fence_ops are values that are nullable. This should probably just use the safe pin_init::zeroed() function. > +impl DmaFence { > + /// Create an initializer for a new [`DmaFence`]. > + fn new( > + fctx: Arc, > + data: impl PinInit, // TODO: The driver data should implement PinInit > + lock: &Opaque, > + context: u64, > + seqno: u64, > + ) -> Result> { This function should be unsafe. There are clearly some safety requirements here. For example, I suspect it's required that `lock` does not get freed before the returned value? Making this function unsafe reveals that DmaFenceCtx::new_fence is missing a safety requirement explaining why self.lock is alive for long enough. Alice