On Mon, 2026-06-08 at 18:43 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> - [Low] Incorrect use of `#[inline(always)]` instead of `#[inline]`
> for abstraction methods.
> --
>
> commit 35068fc880d5bd0d2870750a321a6b8155d32ae4
> Author: Lyude Paul <[email protected]>
>
> rust: drm: gem: shmem: Add DmaResvGuard helper
>
> This commit adds a temporary DmaResvGuard helper type for managing
> the
> dma_resv lock on GEM shmem objects. It implements a drop handler to
> automatically release the lock.
>
> > diff --git a/rust/kernel/drm/gem/shmem.rs
> > b/rust/kernel/drm/gem/shmem.rs
> > index 34af402899a0e..79ebd746e9611 100644
> > --- a/rust/kernel/drm/gem/shmem.rs
> > +++ b/rust/kernel/drm/gem/shmem.rs
>
> [ ... ]
>
> > @@ -235,3 +241,32 @@ 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>,
> > + NotThreadSafe,
> > +);
> > +
> > +impl<'a, T: DriverObject, C: DeviceContext> DmaResvGuard<'a, T, C>
> > {
> > + #[inline(always)]
>
> [Severity: Low]
> This isn't a bug, but should this use #[inline] instead of
> #[inline(always)]?
>
> According to the Rust subsystem guidelines, #[inline(always)] should
> be
> reserved for functions using build_assert!() that depend on function
> parameters. For small abstraction functions that simply forward to
> binding
> calls like this one, #[inline] is the correct annotation.
Looks like the magic 8 ball is either hallucinating or this needs to be
added to the documentation, because there's clearly nothing about this
in the documentation. Ugh.
If anyone can correct me here let me know, otherwise I just assume
hallucination from the hallucination machine
>
> > + #[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, NotThreadSafe)
> > + }
> > +}
> > +
> > +impl<'a, T: DriverObject, C: DeviceContext> Drop for
> > DmaResvGuard<'a, T, C> {
> > + #[inline(always)]
>
> [Severity: Low]
> This isn't a bug, but could this drop method also use #[inline]
> instead of
> #[inline(always)] for the same reason mentioned above?
>
> > + 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())
> > };
> > + }
> > +}
