[RFC PATCH v7 5/8] rust: DmaFence: Replace is_signaled() with status()

[Philipp Stanner]

As it turns out, our dma_fence abstractions lack an essential feature
that is partially derived from the C backend:

Despite it being a common pattern in the kernel, you actually cannot
just check whether a fence was signaled and call it a day - it is
crucial that you also check whether a fence was signaled *with an
error*.

Making things more complicated, an error can get modified at any point
in time before a fence gets signaled. Thus, the only solid way of
obtaining the error code is by simultaneously checking the
signaled-status under lock-protection.

Since the fence status is essential information, replace
Fence::is_signaled() with Fence::status(), making it impossible to
ignore the error code.

Signed-off-by: Philipp Stanner <phasta@xxxxxxxxxx>
---
 rust/helpers/dma_fence.c         |  4 +-
 rust/kernel/dma_buf/dma_fence.rs | 70 +++++++++++++++++++-------------
 rust/kernel/dma_buf/mod.rs       |  3 +-
 3 files changed, 45 insertions(+), 32 deletions(-)

diff --git a/rust/helpers/dma_fence.c b/rust/helpers/dma_fence.c
index 549f6b6a7171..9d8eb7db19bf 100644
--- a/rust/helpers/dma_fence.c
+++ b/rust/helpers/dma_fence.c
@@ -22,9 +22,9 @@ __rust_helper void rust_helper_dma_fence_end_signalling(bool cookie)
 	dma_fence_end_signalling(cookie);
 }
 
-__rust_helper bool rust_helper_dma_fence_is_signaled(struct dma_fence *f)
+__rust_helper bool rust_helper_dma_fence_is_signaled_locked(struct dma_fence *f)
 {
-	return dma_fence_is_signaled(f);
+	return dma_fence_is_signaled_locked(f);
 }
 
 __rust_helper bool rust_helper_dma_fence_test_signaled_flag(struct dma_fence *f)
diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
index d26cf9e8e4b1..71965b26e148 100644
--- a/rust/kernel/dma_buf/dma_fence.rs
+++ b/rust/kernel/dma_buf/dma_fence.rs
@@ -502,40 +502,48 @@ unsafe impl Send for Fence {}
 // SAFETY: Fences are literally designed to be shared between threads.
 unsafe impl Sync for Fence {}
 
+/// Fence-Status queried through [`Fence::status()`].
+#[derive(Debug)]
+pub enum FenceStatus {
+    /// The fence is not yet signaled.
+    Unsignaled,
+    /// The fence is signaled. Might have been signaled with an error.
+    Signaled(Result),
+}
+
 impl Fence {
+    /// Require a fence's status while holding the lock.
+    ///
+    /// # Safety
+    ///
+    /// The dma_fence lock must be held when calling this function.
+    unsafe fn status_locked(&self) -> FenceStatus {
+        // SAFETY: Inner `fence` is valid because `self` is valid.
+        let signaled = unsafe { bindings::dma_fence_is_signaled_locked(self.as_raw()) };
+
+        if signaled {
+            // SAFETY: `self` is valid and we acquired the lock above.
+            let err = unsafe { (*self.as_raw()).error };
+
+            return FenceStatus::Signaled(to_result(err));
+        }
+
+        return FenceStatus::Unsignaled;
+    }
+
     /// Check whether the fence was signaled at the moment of the function call.
+    /// If it is signaled, return the fence's status, i.e., determine if it was
+    /// signaled with an error.
     ///
     /// Note that this can return `true` for a [`Fence`] whose [`DriverFence`]
     /// has not yet been dropped. The reason is that the fence ops callbacks can
     /// cause the fence to get signaled by the C backend.
     #[inline]
-    pub fn is_signaled(&self) -> bool {
-        // We should not use `dma_fence_is_signaled_locked()` here, because
-        // according to the C backend's recommendations, that function is
-        // problematic and we should avoid calling that function with a lock
-        // held.
+    pub fn status(&self) -> FenceStatus {
+        let fence = self.lock();
 
-        // SAFETY: Inner `fence` is valid because `self` is valid.
-        let ret = unsafe { bindings::dma_fence_is_signaled(self.as_raw()) };
-
-        // To be as robust as possible for the future we guarantee that an API
-        // caller can 100% rely on the signaling being completed (i.e., all
-        // fence callbacks ran), so we have to take the lock.
-        //
-        // The reason is that the C dma_fence backend currently does not
-        // carefully synchronize the `dma_fence_is_signaled()` function with the
-        // proper spinlock. This can lead to the function returning `true` while
-        // fence callbacks are still being executed. This can be mitigated by
-        // guarding the entire function with the spinlock.
-        //
-        // The fundamental reason is that the C backend currently does guard
-        // setting of the fence's signaled-bit with the fence's spinlock, but
-        // reading is done locklessly.
-        //
-        // See commit c8a5d5ea3ba6a.
-        let _ = self.lock();
-
-        ret
+        // SAFETY: We required the lock directly above.
+        unsafe { fence.status_locked() }
     }
 
     /// Lock the fence. A helper only to be used internally in this module.
@@ -657,11 +665,15 @@ struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> {
 ///         FenceContextOps,
 ///         FenceCallback,
 ///         FenceCallbackRegistration,
+///         FenceStatus, //
 ///     },
 ///     str::CString,
 ///     sync::aref::ARef, //
 /// };
-/// use core::fmt::Display;
+/// use core::{
+///     assert_matches,
+///     fmt::Display, //
+/// };
 ///
 /// struct CallbackData { }
 ///
@@ -708,9 +720,9 @@ struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> {
 /// let cb_reg = KBox::pin_init(cb_reg, GFP_KERNEL)?;
 ///
 /// // TODO signalling guards
-/// assert_eq!(waiting_fence.is_signaled(), false);
+/// assert_matches!(waiting_fence.status(), FenceStatus::Unsignaled);
 /// fence.signal(Ok(()));
-/// assert_eq!(waiting_fence.is_signaled(), true);
+/// assert_matches!(waiting_fence.status(), FenceStatus::Signaled(Ok(())));
 ///
 /// Ok::<(), Error>(())
 /// ```
diff --git a/rust/kernel/dma_buf/mod.rs b/rust/kernel/dma_buf/mod.rs
index 4764a828642e..b5254144f69e 100644
--- a/rust/kernel/dma_buf/mod.rs
+++ b/rust/kernel/dma_buf/mod.rs
@@ -10,5 +10,6 @@
     FenceCallback,
     FenceCallbackRegistration,
     FenceContext,
-    FenceContextOps, //
+    FenceContextOps,
+    FenceStatus, //
 };
-- 
2.56.0