From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 0D2A9E7E0C3 for ; Mon, 9 Feb 2026 11:30:30 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 6AD7010E3C5; Mon, 9 Feb 2026 11:30:29 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=google.com header.i=@google.com header.b="LkYYdInN"; dkim-atps=neutral Received: from mail-wr1-f74.google.com (mail-wr1-f74.google.com [209.85.221.74]) by gabe.freedesktop.org (Postfix) with ESMTPS id 5C0E310E3C5 for ; Mon, 9 Feb 2026 11:30:28 +0000 (UTC) Received: by mail-wr1-f74.google.com with SMTP id ffacd0b85a97d-437681ecd32so681543f8f.1 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=lists.freedesktop.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=LkYYdInN7hLTTnw9NqMDd06n2pFUuOSgzK+BE/lkKOB4FVBJfDFgIR9Kb8eGkeSefD GF7NFe66diqCas0lQgGhdVBkH8ziH4x5t9KDi9IIj89Eh8Fz7rh5bLLs6LWcdhaZLZrI Fl0ZDc16VdrIheY+Vp/o8b/EYi/beKLY2jESbXVMUYmy5krF70sD1TfvenHouLE/82cb pchJoFDUb6sbEogxhbKrPWq6eLDdf6XL07o4EsWMPeP8uSBJN4kZQqE6JOcZm5cGBaG7 fsP/an0Jed4XP91Nl0+bT8R2baIdRoAKVNclP4b/5MVCyD/jS6+jWiGGdfpgjs4ymsBu QlUQ== 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=nuMzcYzRLWdpe/UW+DPMwUejsQl9zPiqEpmCbcdqjlVphyQyHnFPZFaxaXTwci7mGH /xr0stuRTlhm54WkiUQkagRae0j460SE0HaHEy7ZfAUWdmR16i2PpCXaaB/Zu1Cg1aXp 6kVmEBwD4V03Vn4UXz2p0fROMm5seF0nBpXGy5+0mEVNQX6k1B0l4Sb0F/m+q/gMyRX8 ccLPufMB07BWZP2j/aDJs/Kran75E5UWSH/WMRPELoRNhnvNJYWeh2xzavpdvnzrxD44 M5IcNqXH7qMf4IV/hZsGvMLoWX2ZQ/GjR586bc8MocmN1jj5EkVYmf5F5JIDZ3byOSRR 3riw== X-Forwarded-Encrypted: i=1; AJvYcCVsxRbO+5zUNKHdKKvmqS9UorjDD7Kea/1TvdNOjPuad0t/pSlAF8T2nhBTpePYc5wRtXCO0nux5iQ=@lists.freedesktop.org X-Gm-Message-State: AOJu0YwAmb6WniNwnoXEkZhsywpuS+j02WVVTPx7RM7YsBKQuLhmdxvC WvovuPaK5wZQbe/kzWVtPVX/cLZhtKmrOBrQAk0rCpDT6mOWy9bn+vrHV8X5DKOooAZHzXh3COW QSqiROct3zI04e/+VVg== 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> 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" X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" 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