Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] The `Send` implementation for `GpuVmBo` is missing `T::Object: Sync` and `T::Object: Send` bounds. --
commit 0b3e6c9417da8e3bad6e8397aefb1b71a9755123 Author: Sami Tolvanen <[email protected]> rust: drm: gpuvm: implement Send and Sync for GpuVaAlloc and GpuVmBo This commit implements Send and Sync traits for GpuVaAlloc and GpuVmBo. It asserts that these objects can be safely moved between threads by providing unsafe impls with bounds for the underlying data types. > diff --git a/rust/kernel/drm/gpuvm/vm_bo.rs b/rust/kernel/drm/gpuvm/vm_bo.rs > index c064ac63897be..016b10e3305b8 100644 > --- a/rust/kernel/drm/gpuvm/vm_bo.rs > +++ b/rust/kernel/drm/gpuvm/vm_bo.rs [ ... ] > @@ -19,6 +19,21 @@ pub struct GpuVmBo<T: DriverGpuVm> { > data: T::VmBoData, > } > > +// SAFETY: The refcount in `self.inner` is atomic, so `dec_ref`'s deferred > put is sound from any > +// thread. `data` is dropped later by `drm_gpuvm_bo_deferred_cleanup`, on > whichever thread drains > +// the queue, hence the `T::VmBoData: Send` bound. > +unsafe impl<T: DriverGpuVm> Send for GpuVmBo<T> where T::VmBoData: Send {} [Severity: High] Is this implementation missing bounds for T::Object: Sync and T::Object: Send? Since GpuVmBo acts as a refcounted handle to a GEM object and hands out shared references to it via the obj() method, sending a GpuVmBo to another thread allows that thread to obtain a shared reference concurrently. This requires T::Object: Sync. Additionally, when the deferred cleanup queue is drained, dropping the C-side refcount could result in the final drop of the GEM object occurring on that thread. This requires T::Object: Send. Could omitting these bounds allow safe Rust code to bypass thread-safety guarantees, potentially leading to data races when non-thread-safe GEM objects are shared? > + > +// SAFETY: The fields of `inner` read by shared-reference methods are > immutable after construction. > +// [`Self::data`] hands out `&T::VmBoData` and [`Self::obj`] hands out > `&T::Object`, so sharing > +// `&Self` across threads requires both to be `Sync`. > +unsafe impl<T: DriverGpuVm> Sync for GpuVmBo<T> > +where > + T::VmBoData: Sync, > + T::Object: Sync, > +{ > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
