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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] rust: dma_fence: missing #[inline] on Fence::status() and
  Fence::__status()
--

commit a2517bd1921c87adc707463adc2934165a311348
Author: Philipp Stanner <phasta@kernel.org>

rust: DmaFence: Replace is_signaled() with status()

This commit replaces Fence::is_signaled() with Fence::status() and
introduces FenceStatus so callers check whether a signaled fence completed
with an error under the fence lock.

> diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
> index f74119b4d7f87..b0c0e32462aeb 100644
> --- a/rust/kernel/dma_buf/dma_fence.rs
> +++ b/rust/kernel/dma_buf/dma_fence.rs
> @@ -504,40 +504,49 @@ 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 {
> -    /// Check whether the fence was signaled at the moment of the function call.
> +    /// Query a fence's error code and turn it into a [`Result`].
>      ///
> -    /// 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 {
[ ... ]
> +    /// # Safety
> +    ///
> +    /// The caller must only call this on fences that are already signaled.
> +    unsafe fn __status(&self) -> Result {
> +        // SAFETY: Safe by the function's safety requirements.
> +        let res = unsafe { (*self.as_raw()).error };
>  
> -        ret
> +        to_result(res)
> +    }
> +
> +    /// Query a fence's status at the moment of the function call.
> +    ///
> +    /// If it is signaled, return the fence's result, i.e., determine if it was
> +    /// signaled with an error.
> +    pub fn status(&self) -> FenceStatus {
> +        // The C backend provides `dma_fence_is_signaled()`. However, this is
> +        // currently racy and could lead to misordering with our reading of the
> +        // fence error code. Take the lock to prevent that.
> +        let fence = self.lock();
> +        // SAFETY: `fence` is valid because `self` is valid. The lock was
> +        // taken above.
> +        let signaled = unsafe { bindings::dma_fence_is_signaled_locked(fence.as_raw()) };
> +
> +        if signaled {
> +            // SAFETY: The fence is signaled.
> +            let status = unsafe { fence.__status() };
> +
> +            return FenceStatus::Signaled(status);
> +        }
> +
> +        FenceStatus::Unsignaled
>      }

[Severity: Low]
Should Fence::__status() and Fence::status() retain the #[inline] attribute
that Fence::is_signaled() had, so downstream crates can inline these small
C-binding wrappers across crate boundaries?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106140752.520386-2-phasta@kernel.org?part=5