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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] DmaResvGuard implicitly implements Send, violating kernel mutex semantics.
- [Low] Incorrect usage of `#[inline(always)]` instead of `#[inline]` on abstraction methods.
--

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

rust: drm: gem/shmem: Add DmaResvGuard helper

This commit introduces a temporary DmaResvGuard helper struct to make
locking and unlocking the dma_resv lock for GEM shmem objects easier.

> diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
> index 8b7de136ab1f9..a116b0752e46a 100644
> --- a/rust/kernel/drm/gem/shmem.rs
> +++ b/rust/kernel/drm/gem/shmem.rs

[ ... ]

> @@ -233,3 +236,29 @@ impl<T: DriverObject> driver::AllocImpl for Object<T> {
>          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>(&'a Object<T>);

[Severity: High]
Does this implicitly implement the Send trait if Object<T> implements Sync?

Since this guard drops a kernel ww_mutex in its Drop implementation, moving
the guard to a different thread and unlocking it there would violate the
kernel mutex API contracts. The task that acquires the lock must be the same
task that releases it, otherwise it triggers a kernel warning or panic.

Should this struct include a PhantomData<*mut ()> or
kernel::types::NotThreadSafe marker to explicitly opt out of being Send?

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

[Severity: Low]
This isn't a bug, but does this require the inline(always) attribute?

According to the Rust subsystem coding guidelines, inline(always) should
only be used for functions utilizing build_assert!() that depend on
function parameters. Small abstractions that merely forward to C bindings
should use the standard inline attribute.

> +    #[expect(unused)]
> +    fn new(obj: &'a Object<T>) -> 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> Drop for DmaResvGuard<'a, T> {
> +    #[inline(always)]

[Severity: Low]
This isn't a bug, but similarly here, could this use the standard inline
attribute 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/20260529183702.677677-1-lyude@redhat.com?part=2