Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Low] The patch misses updating `rust/kernel/pwm.rs` for the `AlwaysRefCounted` trait split, resulting in a compilation failure. - [Low] Lingering `AlwaysRefCounted` trait bounds in `aref.rs` and `workqueue.rs` artificially restrict `ARef<T>` functionality for types that only implement `RefCounted`. --
commit 821007fa381542ee45e7b6c5b8f66ecf2e17afcd Author: Oliver Mangold <[email protected]> rust: rename AlwaysRefCounted to RefCounted. This commit splits the AlwaysRefCounted trait into RefCounted (which holds inc_ref and dec_ref) and an empty marker trait AlwaysRefCounted. It updates various implementers across the kernel to use the new traits. > diff --git a/rust/kernel/sync/aref.rs b/rust/kernel/sync/aref.rs > index 4ee5fac0e0b67..2d656f672b97c 100644 > --- a/rust/kernel/sync/aref.rs > +++ b/rust/kernel/sync/aref.rs > @@ -40,9 +38,8 @@ > /// at least until matching decrements are performed. > /// > /// Implementers must also ensure that all instances are reference-counted. > (Otherwise they > -/// won't be able to honour the requirement that > [`AlwaysRefCounted::inc_ref`] keep the object > -/// alive.) > -pub unsafe trait AlwaysRefCounted { > +/// won't be able to honour the requirement that [`RefCounted::inc_ref`] > keep the object alive.) > +pub unsafe trait RefCounted { [Severity: Low] Does rust/kernel/pwm.rs need to be updated for this trait split? It appears Chip<T> still implements AlwaysRefCounted and attempts to define inc_ref and dec_ref within that block, which are no longer part of the trait and will cause a compilation failure: unsafe impl<T: PwmOps> AlwaysRefCounted for Chip<T> { #[inline] fn inc_ref(&self) { [ ... ] > @@ -70,7 +83,7 @@ pub unsafe trait AlwaysRefCounted { > /// > /// The pointer stored in `ptr` is non-null and valid for the lifetime of > the [`ARef`] instance. In > /// particular, the [`ARef`] instance owns an increment on the underlying > object's reference count. > -pub struct ARef<T: AlwaysRefCounted> { > +pub struct ARef<T: RefCounted> { > ptr: NonNull<T>, > _p: PhantomData<T>, > } [Severity: Low] Since ARef now only requires T: RefCounted, should we also relax the trait bounds for ARef in rust/kernel/sync/aref.rs for PartialEq and Eq? They still explicitly bind T: AlwaysRefCounted, which prevents equality checks on ARef wrappers of types that are only RefCounted: impl<T: AlwaysRefCounted + PartialEq<U>, U: AlwaysRefCounted> PartialEq<ARef<U>> for ARef<T> { Similarly, should the trait bounds in rust/kernel/workqueue.rs for WorkItemPointer, RawWorkItem, and RawDelayedWorkItem be relaxed? They currently enforce the old bound, which prevents workqueue scheduling via ARef for types that are only RefCounted: unsafe impl<T, ID> WorkItemPointer<ID> for ARef<T> where T: AlwaysRefCounted, -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
