Re: [PATCH v17 2/6] rust: drm: gem: shmem: Add DmaResvGuard helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] `DmaResvGuard` implicitly implements `Send`, allowing the lock to be sent to and unlocked by a different thread, violating kernel locking rules.
- [Medium] `DmaResvGuard` lacks the `#[must_use]` attribute, which can lead to accidental immediate lock drops.
- [Low] Incorrect use of `#[inline(always)]` instead of `#[inline]` on abstraction functions.
--

commit 35ca0f9ce4df1005935e123463429a164cf640a5
Author: Lyude Paul <lyude@redhat.com>

rust: drm: gem: shmem: Add DmaResvGuard helper

Just a temporary holdover to make locking/unlocking the dma_resv lock much
easier.

> diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
> index 084b798ce795b..650c34dd8b7a4 100644
> --- a/rust/kernel/drm/gem/shmem.rs
> +++ b/rust/kernel/drm/gem/shmem.rs

[ ... ]

> @@ -244,3 +247,29 @@ impl<T: DriverObject, C: DeviceContext> driver::AllocImpl for Object<T, C> {
>          dumb_map_offset: None,
>      };
>  }
> +
> +/// Private helper-type for holding the `dma_resv` object for a GEM shmem object.
> +///
> +/// When this is dropped, the `dma_resv` lock is dropped as well.
> +///
> +// TODO: This should be replace with a WwMutex equivalent once we have such bindings in the kernel.
> +struct DmaResvGuard<'a, T: DriverObject, C: DeviceContext = Registered>(&'a Object<T, C>);

[Severity: High]
Does `DmaResvGuard` implicitly implement `Send`, allowing the lock to be
acquired in one thread, sent to another thread, and unlocked there?

Since `Object<T, C>` implements `Sync`, the shared reference `&'a Object<T, C>`
is `Send`. However, `bindings::dma_resv_unlock` internally calls
`ww_mutex_unlock()`, which requires the mutex to be unlocked by the exact
same task that locked it. 

Should this struct include `NotThreadSafe` (or `PhantomData<*mut ()>`) to
ensure it cannot be sent across threads?

[Severity: Medium]
Could this struct also be annotated with `#[must_use]`? 

Without it, if a caller writes `DmaResvGuard::new(obj);` instead of 
`let _guard = DmaResvGuard::new(obj);`, the lock is acquired and immediately
released at the end of the statement, leaving the rest of the scope
unprotected without triggering a compiler warning.

> +
> +impl<'a, T: DriverObject, C: DeviceContext> DmaResvGuard<'a, T, C> {
> +    #[inline(always)]

[Severity: Low]
This isn't a bug, but according to the Rust coding guidelines, shouldn't small
abstraction functions or binding wrappers like this be annotated with `#[inline]`
rather than `#[inline(always)]`? 

`#[inline(always)]` is generally reserved for functions using `build_assert!()`
that depend on function parameters.

> +    #[expect(unused)]
> +    fn new(obj: &'a Object<T, C>) -> Self {
> +        // SAFETY: This lock is initialized throughout the lifetime of `object`.
> +        unsafe { bindings::dma_resv_lock(obj.raw_dma_resv(), ptr::null_mut()) };
> +
> +        Self(obj)
> +    }
> +}
> +
> +impl<'a, T: DriverObject, C: DeviceContext> Drop for DmaResvGuard<'a, T, C> {
> +    #[inline(always)]

[Severity: Low]
This isn't a bug, but similar to the new() function above, should this
be `#[inline]` instead of `#[inline(always)]`?

> +    fn drop(&mut self) {
> +        // SAFETY: We are releasing the lock grabbed during the creation of this object.
> +        unsafe { bindings::dma_resv_unlock(self.0.raw_dma_resv()) };
> +    }
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603195210.693856-1-lyude@redhat.com?part=2