Re: [PATCH] rust: mem: add DropGuard

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] rust: missing #[inline] annotations on DropGuard methods
- [Low] serdev: compilation error due to method call syntax on associated function
- [Low] rust: non-compliant vertical import style in mem.rs
--

commit 3aca124644cba985e548a767bb3361277176f547
Author: Mohamed Osama <mohamed.osama189110@gmail.com>

rust: mem: add DropGuard

Add a DropGuard type to the Rust kernel memory module for running
cleanup code when a scope is left. It runs a FnOnce callback when dropped
and provides dismiss() to take the wrapped value without running the callback.
Users in gen_disk, serdev, and sync::lock are updated to use DropGuard.

Suggested-by: Gary Guo <gary@garyguo.net>
Link: https://github.com/Rust-for-Linux/linux/issues/1255
Signed-off-by: Mohamed Osama <mohamed.osama189110@gmail.com>

> diff --git a/rust/kernel/mem.rs b/rust/kernel/mem.rs
> index f2d4cdf87d00b..e098675ea1957 100644
> --- a/rust/kernel/mem.rs
> +++ b/rust/kernel/mem.rs
> @@ -4,6 +4,88 @@
>  
>  use crate::prelude::*;
>  
> +use core::mem::ManuallyDrop;
> +use core::ops::{Deref, DerefMut};

[Severity: Low]
Does this deviate from the required vertical block formatting?

The newly added imports in rust/kernel/mem.rs are horizontal and
split across multiple statements. The Rust subsystem guidelines mandate that
commits touching imports should follow the vertical import style (e.g.,
grouping them vertically under a single use core::{...}; block).

> +
> +/// Wraps a value and runs a closure when dropped.
> +///
> +/// This is useful for running cleanup code when leaving a scope.
> +///
> +/// The [`DropGuard::dismiss`] function can be used to take ownership of the wrapped
> +/// value without running the cleanup function.
> +#[doc(alias = "ScopeGuard")]
> +#[doc(alias = "defer")]
> +pub struct DropGuard<T, F>
> +where
> +    F: FnOnce(T),
> +{
> +    inner: ManuallyDrop<T>,
> +    f: ManuallyDrop<F>,
> +}
> +
> +impl<T, F> DropGuard<T, F>
> +where
> +    F: FnOnce(T),
> +{
> +    /// Creates a new `DropGuard`.
> +    #[must_use]
> +    pub fn new(inner: T, f: F) -> Self {

[Severity: Low]
Should small wrapper functions in abstractions like the kernel crate use
#[inline]?

The DropGuard::new() constructor lacks this annotation. Missing #[inline]
annotations can prevent the compiler from optimizing away wrapper overhead
when calling small wrapper methods.

> +        Self {
> +            inner: ManuallyDrop::new(inner),
> +            f: ManuallyDrop::new(f),
> +        }
> +    }
> +
> +    /// Consumes the `DropGuard`, returning the wrapped value without
> +    /// running the cleanup function.
> +    pub fn dismiss(guard: Self) -> T {

[Severity: Low]
Is this trivial state-transition function missing the #[inline] annotation
as well?

The DropGuard::dismiss() function in rust/kernel/mem.rs lacks #[inline],
which may prevent optimization.

> +        let mut guard = ManuallyDrop::new(guard);
> +
> +        // SAFETY: We have taken ownership of the guard and prevent its destructor from running.
> +        let value = unsafe { ManuallyDrop::take(&mut guard.inner) };
> +
> +        // SAFETY: We have taken ownership of the guard.
> +        unsafe { ManuallyDrop::drop(&mut guard.f) };
> +
> +        value
> +    }
> +}
> +
> +impl<T, F> Deref for DropGuard<T, F>
> +where
> +    F: FnOnce(T),
> +{
> +    type Target = T;
> +
> +    fn deref(&self) -> &T {

[Severity: Low]
Should #[inline] be added to this trivial trait method?

The DropGuard::deref() method is a thin wrapper and missing #[inline].

> +        &self.inner
> +    }
> +}
> +
> +impl<T, F> DerefMut for DropGuard<T, F>
> +where
> +    F: FnOnce(T),
> +{
> +    fn deref_mut(&mut self) -> &mut T {

[Severity: Low]
Should #[inline] be added to deref_mut() for the same reason?

The DropGuard::deref_mut() method in rust/kernel/mem.rs is missing
#[inline].

> +        &mut self.inner
> +    }
> +}
> +
> +impl<T, F> Drop for DropGuard<T, F>
> +where
> +    F: FnOnce(T),
> +{
> +    fn drop(&mut self) {

[Severity: Low]
Does this drop implementation acting as a thin wrapper also need #[inline]?

The DropGuard::drop() method lacks #[inline], which may prevent optimization
when dropping the guard.

[ ... ]

> diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
> index 17ca504b7f8d6..a400d241c2ad6 100644
> --- a/rust/kernel/serdev.rs
> +++ b/rust/kernel/serdev.rs

[ ... ]

> @@ -174,7 +172,7 @@ extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi:
>              }))?;
>              // SAFETY: We just set drvdata to `PrivateData<'_, T>`.
>              let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() };
> -            let private_data = ScopeGuard::new_with_data(private_data, |_| {
> +            let private_data = DropGuard::new(private_data, |_| {
>                  // SAFETY: We just set drvdata to `PrivateData<'_, T>`.
>                  drop(unsafe { sdev.as_ref().drvdata_obtain::<PrivateData<'_, T>>() });
>              });

[Severity: Low]
Does this break the build when CONFIG_RUST_SERDEV is enabled?

The patch replaces ScopeGuard with DropGuard, but further down in
probe_callback() in rust/kernel/serdev.rs, there is an unmodified call to
private_data.dismiss():

rust/kernel/serdev.rs:probe_callback() {
...
            result.map(|()| {
                private_data.dismiss();
                0
            })
}

Since DropGuard::dismiss() is an associated function rather than a method
taking self, private_data.dismiss() is now invalid method call syntax and
fails to compile. Should this call be updated to
DropGuard::dismiss(private_data)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916011500.529743-1-mohamed.osama189110@gmail.com?part=1