Re: [PATCH v2] 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 method call on DropGuard
- [Low] rust: mem: DropGuard abstraction traits missing #[inline]
--

commit e6da4b253a1c111afae2f6a79a8b6b1616c3e342
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. `DropGuard` runs a `FnOnce` callback 
when dropped and provides `dismiss()` to take the wrapped value without 
running the callback.

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 {
       ^^^^^^^^

[Severity: Low]
Does this small abstraction method need an #[inline] annotation?

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

[Severity: Low]
Should this method also be annotated with #[inline]?

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

[Severity: Low]
Should we add #[inline] to this method as well?

> +        // SAFETY: `DropGuard` is in the process of being dropped.
> +        let inner = unsafe { ManuallyDrop::take(&mut self.inner) };
> +
> +        // SAFETY: `DropGuard` is in the process of being dropped.
> +        let f = unsafe { ManuallyDrop::take(&mut self.f) };
> +
> +        f(inner);
> +    }
> +}
[ ... ]
> 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 change cause a compilation failure further down in probe_callback()?

Because private_data is now a DropGuard, it no longer has a dismiss() method
that takes self. Later in the function we have:

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

Could we change this to use the associated function syntax instead, like
DropGuard::dismiss(private_data)?

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