[PATCH] rust: DmaFence: Add better warning through Device reference

[PATCH] rust: DmaFence: Add better warning through Device reference

[Philipp Stanner]

FenceContext::drop() contains a warning print that warns about possible
memory corruptions if there are forgotten fences. However, precisely
speaking, a forgotten fence is undefined behavior.

Moreover, forgotten fences hint at a severe design problem in the
driver. In this context, the used pr_err!() does not provide very useful
text output.

Replace the warning print with a dev_warn!(). To do so, have the
FenceContext carry a reference to a Device, protected by the already
present lifetime.

Suggested-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 rust/kernel/dma_buf/dma_fence.rs | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
index 18a43e1bb442..58ba678e91b5 100644
--- a/rust/kernel/dma_buf/dma_fence.rs
+++ b/rust/kernel/dma_buf/dma_fence.rs
@@ -32,6 +32,10 @@
 };
 
 use kernel::{
+    device::{
+        Device,
+        Normal, //
+    },
     str::CString,
     sync::{
         aref::{
@@ -59,7 +63,7 @@ pub trait FenceContextOps {
 /// with each other, providing each with raising sequence numbers and a common
 /// identifier.
 #[pin_data(PinnedDrop)]
-pub struct FenceContext<T: FenceContextOps + Send + Sync> {
+pub struct FenceContext<'a, T: FenceContextOps + Send + Sync> {
     /// The fence context number.
     nr: u64,
     /// The sequence number for the next fence created.
@@ -81,12 +85,14 @@ pub struct FenceContext<T: FenceContextOps + Send + Sync> {
     // the lifetime which intends to enforce that all fences disappear before
     // their context.
     nr_of_unsignaled_fences: Atomic<usize>,
+    /// The device this fence context is associated with.
+    dev: &'a Device<Normal>,
     /// The user's data.
     #[pin]
     data: T,
 }
 
-impl<'a, T: Send + Sync + FenceContextOps> FenceContext<T> {
+impl<'a, T: Send + Sync + FenceContextOps + 'a> FenceContext<'a, T> {
     // This can later be extended as a vtable in case other parties need support
     // for the more "exotic" callbacks.
     const OPS: bindings::dma_fence_ops = bindings::dma_fence_ops {
@@ -106,6 +112,7 @@ pub fn new<E>(
         initial_seqno: u64,
         driver_name: &CStr,
         timeline_name: &CStr,
+        dev: &'a Device<Normal>,
         data: impl PinInit<T, E>,
     ) -> impl PinInit<Self, Error>
     where
@@ -122,6 +129,7 @@ pub fn new<E>(
             driver_name: driver_name?,
             timeline_name: timeline_name?,
             nr_of_unsignaled_fences: Atomic::new(0),
+            dev,
             data <- data,
         })
     }
@@ -211,7 +219,7 @@ unsafe fn from_raw_fence(ptr: *mut bindings::dma_fence) -> &'a Self {
 }
 
 #[pinned_drop]
-impl<T: FenceContextOps + Send + Sync> PinnedDrop for FenceContext<T> {
+impl<T: FenceContextOps + Send + Sync> PinnedDrop for FenceContext<'_, T> {
     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
@@ -596,7 +604,7 @@ struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> {
     /// Callback head for dropping this in a deferred manner through RCU.
     rcu_head: bindings::callback_head,
     /// Reference to access the FenceContext.
-    fctx: &'a FenceContext<T>,
+    fctx: &'a FenceContext<'a, T>,
     /// The API user's data. It is essential that the data only performs
     /// operations legal in atomic context in its [`Drop`] implementation.
     #[pin]
@@ -641,6 +649,7 @@ struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> {
 ///
 /// ```
 /// use kernel::{
+///     device::Normal,
 ///     dma_buf::{
 ///         DriverFence,
 ///         FenceContext,
@@ -648,6 +657,7 @@ struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> {
 ///         FenceCallback,
 ///         FenceCallbackRegistration,
 ///     },
+///     faux,
 ///     str::CString,
 ///     sync::aref::ARef, //
 /// };
@@ -676,9 +686,10 @@ struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> {
 ///
 /// let fctx_data = FenceContextData::new();
 ///
+/// let reg = faux::Registration::new(c"DmaFence-KUnit-Test", None)?;
 ///
 /// let mut fctx = KBox::pin_init(
-///     FenceContext::new(0, c"dummy_driver", c"dummy_timeline", fctx_data),
+///     FenceContext::new(0, c"dummy_driver", c"dummy_timeline", reg.as_ref().as_ref(), fctx_data),
 ///     GFP_KERNEL
 /// )?;
 ///
@@ -934,7 +945,9 @@ fn drop(&mut self) {
         // SAFETY: `guard` is valid until the `call_rcu()` below.
         let signaled: bool = unsafe { bindings::dma_fence_test_signaled_flag(guard.as_raw()) };
         if !signaled {
-            pr_err!("DriverFence drops unsignaled. Danger of memory corruption!\n");
+            // SAFETY: `data` is valid because `self` is valid.
+            let dev = unsafe { self.data.as_ref().fctx.dev };
+            dev_warn!(dev, "DriverFence drops unsignaled.\n");
             // SAFETY: `guard` is valid until the `call_rcu()` below. The fence
             // must not have been signaled yet, which we check directly above.
             unsafe { bindings::dma_fence_set_error(guard.as_raw(), ECANCELED.to_errno()) };

base-commit: 896ed083362758b33c49a1b5e5a3423c5814d87e
-- 
2.55.0

Re: [PATCH] rust: DmaFence: Add better warning through Device reference

[Gary Guo]

On Fri Sep 25, 2026 at 9:19 AM BST, Philipp Stanner wrote:
> FenceContext::drop() contains a warning print that warns about possible
> memory corruptions if there are forgotten fences. However, precisely
> speaking, a forgotten fence is undefined behavior.
>
> Moreover, forgotten fences hint at a severe design problem in the
> driver. In this context, the used pr_err!() does not provide very useful
> text output.
>
> Replace the warning print with a dev_warn!(). To do so, have the
> FenceContext carry a reference to a Device, protected by the already
> present lifetime.

Add a lifetime just to do this a print doesn't sound ideal. You could use
`ARef<Device>` instead?

Best,
Gary

>
> Suggested-by: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: Philipp Stanner <phasta@kernel.org>
> ---
>  rust/kernel/dma_buf/dma_fence.rs | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
>

Re: [PATCH] rust: DmaFence: Add better warning through Device reference

[Danilo Krummrich]

On Fri Sep 25, 2026 at 12:26 PM CEST, Gary Guo wrote:
> On Fri Sep 25, 2026 at 9:19 AM BST, Philipp Stanner wrote:
>> Replace the warning print with a dev_warn!(). To do so, have the
>> FenceContext carry a reference to a Device, protected by the already
>> present lifetime.
>
> Add a lifetime just to do this a print doesn't sound ideal. You could use
> `ARef<Device>` instead?

Yes, that's what I recommend in general.

Another thing to consider for the DMA fence context specifically, there's two
kinds of users. The absolute majority are drivers; there are very few non-driver
ones (e.g. drm_crtc), which will remain to be C code for at least quite a while.

I think it wouldn't be unreasonable to keep the API driver centric, and in this
case it also wouldn't be unreasonable to add the lifetime, as the T will most
likely carry it anyway, and even if not it wouldn't make sense for the fence
context to outlive driver unbind.

That said, we can also use WARN_ON() instead, which avoids the device dependency
to begin with and still provides enough information to find the "offender".

But as I mentioned previously, I don't consider this that bad of a condition in
the first place. Signaling with ECANCELED on drop() seems perfectly reasonable:

When a driver does a teardown of the channel (or more generically the execution
context) it will follow the RAII pattern, so it will be very natural to just
drop the Jobqueue, which will drop all jobs and hence all DriverFence objects.

IOW, driver will likely invent a new type that does the same thing on drop, just
without the warning. :)

Re: [PATCH] rust: DmaFence: Add better warning through Device reference

[Philipp Stanner]

On Fri, 2026-09-25 at 14:30 +0200, Danilo Krummrich wrote:
> On Fri Sep 25, 2026 at 12:26 PM CEST, Gary Guo wrote:
> > On Fri Sep 25, 2026 at 9:19 AM BST, Philipp Stanner wrote:
> > > Replace the warning print with a dev_warn!(). To do so, have the
> > > FenceContext carry a reference to a Device, protected by the already
> > > present lifetime.
> > 
> > Add a lifetime just to do this a print doesn't sound ideal. You could use
> > `ARef<Device>` instead?
> 
> Yes, that's what I recommend in general.

Weren't you super opposed to refcounting wherever it's avoidable?


[…]

> That said, we can also use WARN_ON() instead, which avoids the device dependency
> to begin with and still provides enough information to find the "offender".

WARN_ON() is fine by me.

> 
> But as I mentioned previously, I don't consider this that bad of a condition in
> the first place. Signaling with ECANCELED on drop() seems perfectly reasonable:
> 
> When a driver does a teardown of the channel (or more generically the execution
> context) it will follow the RAII pattern, so it will be very natural to just
> drop the Jobqueue, which will drop all jobs and hence all DriverFence objects.
> 
> IOW, driver will likely invent a new type that does the same thing on drop, just
> without the warning. :)

The driver can avoid dropping half-forgotten stuff by calling
jobqueue.complete_all_jobs(ECANCELED) immediately before dropping,
which allows us for having the warning without false-positives.


P.

Re: [PATCH] rust: DmaFence: Add better warning through Device reference

[Danilo Krummrich]

On Fri Sep 25, 2026 at 3:08 PM CEST, Philipp Stanner wrote:
> The driver can avoid dropping half-forgotten stuff by calling
> jobqueue.complete_all_jobs(ECANCELED) immediately before dropping,
> which allows us for having the warning without false-positives.

This would just move it up a layer from inventing a new type around DriverFence
to invent a new type around Jobqueue which does that instead.

But the Jobqueue won't have any procedural teardown, it has to teardown stuff in
drop() anyway, which includes dropping jobs already.

What I'm saying is that there's not much value in DriverFence::drop() having
this warning in the first place, which then either the driver or the jobqueue
has to work around on teardown.

Re: [PATCH] rust: DmaFence: Add better warning through Device reference

[Danilo Krummrich]

On Fri Sep 25, 2026 at 3:08 PM CEST, Philipp Stanner wrote:
> On Fri, 2026-09-25 at 14:30 +0200, Danilo Krummrich wrote:
>> On Fri Sep 25, 2026 at 12:26 PM CEST, Gary Guo wrote:
>> > On Fri Sep 25, 2026 at 9:19 AM BST, Philipp Stanner wrote:
>> > > Replace the warning print with a dev_warn!(). To do so, have the
>> > > FenceContext carry a reference to a Device, protected by the already
>> > > present lifetime.
>> > 
>> > Add a lifetime just to do this a print doesn't sound ideal. You could use
>> > `ARef<Device>` instead?
>> 
>> Yes, that's what I recommend in general.
>
> Weren't you super opposed to refcounting wherever it's avoidable?

Well, a device by itself is already reference counted (which is fine). The
lifetime constraint does not come from the lifetime of a struct device, it comes
from it being bound to a driver for bus devices; for class devices the lifetime
constraint comes from the device being registered.

I'm opposed to refcount stuff that has a bounded lifetime that is defined by
some scope it must not outlive.

Re: [PATCH] rust: DmaFence: Add better warning through Device reference

[Philipp Stanner]

On Fri, 2026-09-25 at 17:50 +0200, Danilo Krummrich wrote:
> On Fri Sep 25, 2026 at 3:08 PM CEST, Philipp Stanner wrote:
> > The driver can avoid dropping half-forgotten stuff by calling
> > jobqueue.complete_all_jobs(ECANCELED) immediately before dropping,
> > which allows us for having the warning without false-positives.
> 
> This would just move it up a layer from inventing a new type around DriverFence
> to invent a new type around Jobqueue which does that instead.
> 
> But the Jobqueue won't have any procedural teardown, it has to teardown stuff in
> drop() anyway, which includes dropping jobs already.
> 
> What I'm saying is that there's not much value in DriverFence::drop() having
> this warning in the first place, which then either the driver or the jobqueue
> has to work around on teardown.

I guess we agree that it would be a horrible bug if there's still a
command buffer running on the GPU that can access memory which might
have been freed once the associated fence signaled.

So I suppose what you are saying is more: there is not much value in
the case of *JobQueue*, basically because all the jobs live inside of
it anyways.

So I suppose we agree that a warning is fine. It won't fire in JQ
anyways, but might benefit others.


P.

Re: [PATCH] rust: DmaFence: Add better warning through Device reference

[Danilo Krummrich]

On Fri Sep 25, 2026 at 7:21 PM CEST, Philipp Stanner wrote:
> On Fri, 2026-09-25 at 17:50 +0200, Danilo Krummrich wrote:
>> On Fri Sep 25, 2026 at 3:08 PM CEST, Philipp Stanner wrote:
>> > The driver can avoid dropping half-forgotten stuff by calling
>> > jobqueue.complete_all_jobs(ECANCELED) immediately before dropping,
>> > which allows us for having the warning without false-positives.
>> 
>> This would just move it up a layer from inventing a new type around DriverFence
>> to invent a new type around Jobqueue which does that instead.
>> 
>> But the Jobqueue won't have any procedural teardown, it has to teardown stuff in
>> drop() anyway, which includes dropping jobs already.
>> 
>> What I'm saying is that there's not much value in DriverFence::drop() having
>> this warning in the first place, which then either the driver or the jobqueue
>> has to work around on teardown.
>
> I guess we agree that it would be a horrible bug if there's still a
> command buffer running on the GPU that can access memory which might
> have been freed once the associated fence signaled.

Sure, but that's unrelated.

> So I suppose what you are saying is more: there is not much value in
> the case of *JobQueue*, basically because all the jobs live inside of
> it anyways.

No, I'm saying there is not much value in general. Whatever thing owns the
DriverFence has to represent the "device access" of some resource, which
already naturally establishes the relationship.

IOW, whatever thing owns a DriverFence is also the thing that stops the hardware
in its own drop() implementation; anything else would be rather questionable.

> So I suppose we agree that a warning is fine. It won't fire in JQ
> anyways, but might benefit others.

What scenario are you thinking of?