Re: [PATCH v1] rust: mem: add DropGuard

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] rust: serdev: compilation error due to invalid dismiss() method syntax
- [Low] rust: mem: missing #[inline] annotations on DropGuard methods
--

commit cb72c5069d5d615bc6c2f05a5b1ca631930446aa
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 replaces the current ScopeGuard
users in gen_disk, serdev, and sync::lock.

Link: https://github.com/Rust-for-Linux/linux/issues/1255

> diff --git a/rust/kernel/mem.rs b/rust/kernel/mem.rs
> index f2d4cdf87d00b..f2d1f9bc93379 100644
> --- a/rust/kernel/mem.rs
> +++ b/rust/kernel/mem.rs
[ ... ]
> +impl<T, F> Deref for DropGuard<T, F>
> +where
> +    F: FnOnce(T),
> +{
> +    type Target = T;
> +
> +    fn deref(&self) -> &T {
> +        &self.inner
> +    }
> +}

[Severity: Low]
Should this function be annotated with #[inline]? According to the Rust
subsystem coding guidelines, small functions in abstractions like the kernel
crate should use #[inline].

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

[Severity: Low]
Does this function also need an #[inline] annotation?

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

[Severity: Low]
Could this drop() implementation also use #[inline] since it is a small
abstraction function?

[ ... ]
> 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]
Is there a missed migration point further down in this function that causes a
compilation error?

In probe_callback(), the variable private_data is now a DropGuard, but later
in the function the code still does this:

            result.map(|()| {
                private_data.dismiss();
                0
            })

Since DropGuard::dismiss is implemented as an associated function rather than
a method taking a self receiver, does the compiler attempt to resolve
dismiss() on the inner type (PrivateData) via Deref and fail to compile when
CONFIG_RUST_SERIAL_DEV_BUS_ABSTRACTIONS is enabled?

Should that be updated to DropGuard::dismiss(private_data) to match the new
API?

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