Re: [PATCH v21 4/9] rust: rename `AlwaysRefCounted` to `RefCounted`.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `ForeignOwnable` implementation for `ARef<T>` unnecessarily requires `T: AlwaysRefCounted` instead of `T: RefCounted`.
- [Low] Conditional compilation failure in `fwctl.rs` and `serdev.rs` due to incomplete `AlwaysRefCounted` refactoring.
--

commit 6b87875ccf108a75e9bf33ac87bec77ac53590b0
Author: Oliver Mangold <oliver.mangold@pm.me>

rust: rename `AlwaysRefCounted` to `RefCounted`.

There are types where it may both be reference counted in some cases and
owned in others. In such cases, obtaining `ARef<T>` from `&T` would be
unsound as it allows creation of `ARef<T>` copy from `&Owned<T>`.

[ ... ]

[Severity: Low]
The tree-wide refactoring seems to have missed `rust/kernel/fwctl.rs` and
`rust/kernel/serdev.rs`, which still attempt to implement methods on
`AlwaysRefCounted`.

For example, in `rust/kernel/fwctl.rs`:

unsafe impl<T: Operations> AlwaysRefCounted for Device<T> {
    #[inline]
    fn inc_ref(&self) {
        // SAFETY: `self` holds a live reference.
        unsafe { bindings::fwctl_get(self.as_raw()) };
    }
    ...

Since `AlwaysRefCounted` is now an empty marker trait, will this cause
compilation failures when `CONFIG_FWCTL` or `CONFIG_SERIAL_DEV_BUS` are
enabled?

> diff --git a/rust/kernel/sync/aref.rs b/rust/kernel/sync/aref.rs
> index 9983ee0852486..215e7eb139fac 100644
> --- a/rust/kernel/sync/aref.rs
> +++ b/rust/kernel/sync/aref.rs
[ ... ]
> @@ -188,15 +201,15 @@ fn drop(&mut self) {
>  
>  impl<T, U> PartialEq<ARef<U>> for ARef<T>
>  where
> -    T: AlwaysRefCounted + PartialEq<U>,
> -    U: AlwaysRefCounted,
> +    T: RefCounted + PartialEq<U>,
> +    U: RefCounted,
>  {
>      #[inline]
>      fn eq(&self, other: &ARef<U>) -> bool {
>          T::eq(&**self, &**other)
>      }
>  }
> -impl<T: AlwaysRefCounted + Eq> Eq for ARef<T> {}
> +impl<T: RefCounted + Eq> Eq for ARef<T> {}
>  
>  // SAFETY: `into_foreign` returns a pointer from `NonNull::as_ptr`, so it's non-null. The
>  // `ARef` invariant guarantees that `ptr` points to a valid `T`, so it's aligned to `T`.

[Severity: Medium]
This trait bound appears to have been missed during the conversion. Just below
this context in `aref.rs` is:

unsafe impl<T: AlwaysRefCounted> ForeignOwnable for ARef<T> {

Does this unnecessarily limit the FFI usability of `ARef<T>` for types that
are `RefCounted` but not `AlwaysRefCounted`? Should this be updated to
`T: RefCounted` instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910-unique-ref-v21-0-e83257373062@kernel.org?part=4