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 <[email protected]> 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/[email protected]?part=4
