All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Danilo Krummrich" <dakr@kernel.org>
To: "Philipp Stanner" <phasta@kernel.org>
Cc: "Miguel Ojeda" <ojeda@kernel.org>,
	"Boqun Feng" <boqun@kernel.org>, "Gary Guo" <gary@garyguo.net>,
	"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
	"Benno Lossin" <lossin@kernel.org>,
	"Andreas Hindborg" <a.hindborg@kernel.org>,
	"Alice Ryhl" <aliceryhl@google.com>,
	"Trevor Gross" <tmgross@umich.edu>,
	"Daniel Almeida" <daniel.almeida@collabora.com>,
	"Tamir Duberstein" <tamird@kernel.org>,
	"Alexandre Courbot" <acourbot@nvidia.com>,
	"Onur Özkan" <work@onurozkan.dev>,
	"Sumit Semwal" <sumit.semwal@linaro.org>,
	"Christian König" <christian.koenig@amd.com>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Yury Norov (NVIDIA)" <yury.norov@gmail.com>,
	"Asahi Lina" <lina+kernel@asahilina.net>,
	"Burak Emir" <burak.emir@gmail.com>,
	"Lorenzo Stoakes" <ljs@kernel.org>,
	"Joel Fernandes" <joelagnelf@nvidia.com>,
	"FUJITA Tomonori" <fujita.tomonori@gmail.com>,
	"Boris Brezillon" <boris.brezillon@collabora.com>,
	linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org,
	linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org,
	linaro-mm-sig@lists.linaro.org
Subject: Re: [PATCH v10 1/2] rust: Add dma_fence abstractions
Date: Fri, 04 Sep 2026 17:32:55 +0200	[thread overview]
Message-ID: <DL6N387CE2AT.1LU4TDUB8QBIU@kernel.org> (raw)
In-Reply-To: <20260812072217.1971807-3-phasta@kernel.org>

On Wed Aug 12, 2026 at 9:22 AM CEST, Philipp Stanner wrote:
> Add abstractions for dma_fence in Rust.

I was about to pick this up, but ended up with too many things to tweak.

  * Fix checkpatch.pl warnings,
  * take &CStr instead of CString in FenceContext::new(),
  * change DriverFenceAllocation::ops to &'static from *const,
  * remove # Safety comment on AlwaysRefCounted::dec_ref() impl,
  * use Opaque::zeroed() instead of __GFP_ZERO,
  * use kernel vertical import style.

Here's the diff I suggest for a v11:

diff --git a/rust/helpers/dma_fence.c b/rust/helpers/dma_fence.c
index 0e08411098fa..549f6b6a7171 100644
--- a/rust/helpers/dma_fence.c
+++ b/rust/helpers/dma_fence.c
@@ -37,7 +37,8 @@ __rust_helper void rust_helper_dma_fence_lock_irqsave(struct dma_fence *f, unsig
 	dma_fence_lock_irqsave(f, *flags);
 }

-__rust_helper void rust_helper_dma_fence_unlock_irqrestore(struct dma_fence *f, unsigned long *flags)
+__rust_helper void rust_helper_dma_fence_unlock_irqrestore(struct dma_fence *f,
+							   unsigned long *flags)
 {
 	dma_fence_unlock_irqrestore(f, *flags);
 }
diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
index f22f4a07c152..d82b69f68e61 100644
--- a/rust/kernel/dma_buf/dma_fence.rs
+++ b/rust/kernel/dma_buf/dma_fence.rs
@@ -102,21 +102,23 @@ impl<'a, T: Send + Sync + FenceContextOps> FenceContext<T> {
     /// Create a new `FenceContext`.
     pub fn new<E>(
         initial_seqno: u64,
-        driver_name: CString,
-        timeline_name: CString,
+        driver_name: &CStr,
+        timeline_name: &CStr,
         data: impl PinInit<T, E>,
     ) -> impl PinInit<Self, Error>
     where
         Error: From<E>,
     {
+        let driver_name = CString::try_from(driver_name);
+        let timeline_name = CString::try_from(timeline_name);
         try_pin_init!(Self {
             // SAFETY: `dma_fence_context_alloc()` merely works on a global
             // atomic. Parameter `1` is the number of contexts we want to
             // allocate.
             nr: unsafe { bindings::dma_fence_context_alloc(1) },
             seqno: Atomic::new(initial_seqno),
-            driver_name,
-            timeline_name,
+            driver_name: driver_name?,
+            timeline_name: timeline_name?,
             nr_of_unsignaled_fences: Atomic::new(0),
             data <- data,
         })
@@ -141,7 +143,7 @@ pub fn new_fence_allocation(
             rcu_head: Default::default(),
             // `inner` remains uninitialized until a `DriverFence` takes over.
             inner: Fence {
-                inner: Opaque::uninit(),
+                inner: Opaque::zeroed(),
             },
             fctx: self,
             data,
@@ -154,7 +156,7 @@ pub fn new_fence_allocation(
         //
         // Hence, we need the manage the memory manually. It will be freed by the
         // C backend automatically once the refcount within `Fence` drops to 0.
-        let data = KBox::new(fence_data, GFP_KERNEL | __GFP_ZERO)?;
+        let data = KBox::new(fence_data, GFP_KERNEL)?;

         Ok(DriverFenceAllocation {
             data,
@@ -212,7 +214,7 @@ fn drop(self: Pin<&mut Self>) {
         // Fence ops callbacks can be called on unsignaled fences. Since these
         // callbacks can access the fence context and its data, it needs to be
         // guaranteed that a context only drops after all associated
-        // `DriverFence`s have been dropped. This is unlikely to ocurr, but would
+        // `DriverFence`s have been dropped. This is unlikely to occur, but would
         // result in silent UAF. Throw a panic to prevent that.
         //
         // TODO:
@@ -568,9 +570,6 @@ fn inc_ref(&self) {
         unsafe { bindings::dma_fence_get(self.as_raw()) }
     }

-    /// # Safety
-    ///
-    /// `ptr`must be a valid pointer to a [`DriverFence`].
     unsafe fn dec_ref(ptr: NonNull<Self>) {
         // SAFETY: `ptr` is never a NULL pointer; and when `dec_ref()` is called
         // the fence is by definition still valid.
@@ -640,15 +639,17 @@ struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> {
 /// # Examples
 ///
 /// ```
-/// use kernel::dma_buf::{
-///     DriverFence,
-///     FenceContext,
-///     FenceContextOps,
-///     FenceCallback,
-///     FenceCallbackRegistration, //
+/// use kernel::{
+///     dma_buf::{
+///         DriverFence,
+///         FenceContext,
+///         FenceContextOps,
+///         FenceCallback,
+///         FenceCallbackRegistration,
+///     },
+///     str::CString,
+///     sync::aref::ARef,
 /// };
-/// use kernel::str::CString;
-/// use kernel::sync::aref::ARef;
 /// use core::fmt::Display;
 ///
 /// struct CallbackData { }
@@ -674,18 +675,16 @@ struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> {
 ///
 /// let fctx_data = FenceContextData::new();
 ///
-/// let driver_name = CString::try_from_fmt(fmt!("dummy_driver"))?;
-/// let timeline_name = CString::try_from_fmt(fmt!("dummy_timeline"))?;
-///
 /// let mut fctx = KBox::pin_init(
-///     FenceContext::new(0, driver_name, timeline_name, fctx_data), GFP_KERNEL)?;
+///     FenceContext::new(0, c"dummy_driver", c"dummy_timeline", fctx_data),
+///     GFP_KERNEL,
+/// )?;
 ///
 /// struct FenceData {
 ///     data: CString,
 /// }
 ///
-/// let data = CString::try_from_fmt(fmt!("dummy_data"))?;
-/// let fence_data = FenceData { data };
+/// let fence_data = FenceData { data: c"dummy_data".try_into()? };
 ///
 /// let fence_alloc = fctx.new_fence_allocation(fence_data)?;
 /// let mut fence = fence_alloc.new_fence();
@@ -718,8 +717,8 @@ pub struct DriverFenceAllocation<'a, T: Send + Sync + FenceContextOps> {
     /// Handed over to a [`DriverFence`], or deallocated once the
     /// [`DriverFenceAllocation`] drops.
     data: KBox<DriverFenceData<'a, T>>,
-    /// Pointer for the ops for the associated [`FenceContext`]
-    ops: *const bindings::dma_fence_ops,
+    /// Reference for the ops for the associated [`FenceContext`]
+    ops: &'static bindings::dma_fence_ops,
 }

 impl<'a, T: Send + Sync + FenceContextOps> DriverFenceAllocation<'a, T> {

  parent reply	other threads:[~2026-09-04 15:33 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-08-12  7:22 [PATCH v10 0/2] rust / dma_buf: Add abstractions for dma_fence Philipp Stanner
2026-08-12  7:22 ` [PATCH v10 1/2] rust: Add dma_fence abstractions Philipp Stanner
2026-08-12  7:38   ` sashiko-bot
2026-09-04 15:32   ` Danilo Krummrich [this message]
2026-09-04 18:53     ` Philipp Stanner
2026-09-04 19:10       ` Danilo Krummrich
2026-08-12  7:22 ` [PATCH v10 2/2] MAINTAINERS: Add entry for Rust dma-buf Philipp Stanner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DL6N387CE2AT.1LU4TDUB8QBIU@kernel.org \
    --to=dakr@kernel.org \
    --cc=a.hindborg@kernel.org \
    --cc=acourbot@nvidia.com \
    --cc=aliceryhl@google.com \
    --cc=bjorn3_gh@protonmail.com \
    --cc=boqun@kernel.org \
    --cc=boris.brezillon@collabora.com \
    --cc=burak.emir@gmail.com \
    --cc=christian.koenig@amd.com \
    --cc=daniel.almeida@collabora.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=fujita.tomonori@gmail.com \
    --cc=gary@garyguo.net \
    --cc=gregkh@linuxfoundation.org \
    --cc=joelagnelf@nvidia.com \
    --cc=lina+kernel@asahilina.net \
    --cc=linaro-mm-sig@lists.linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=ljs@kernel.org \
    --cc=lossin@kernel.org \
    --cc=ojeda@kernel.org \
    --cc=phasta@kernel.org \
    --cc=rust-for-linux@vger.kernel.org \
    --cc=sumit.semwal@linaro.org \
    --cc=tamird@kernel.org \
    --cc=tmgross@umich.edu \
    --cc=work@onurozkan.dev \
    --cc=yury.norov@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.