On Fri Jun 5, 2026 at 4:24 AM JST, Lyude Paul wrote: > In order to do this, we need to be careful to ensure that any interface we > expose for scatterlists ensures that any mappings created from one are > destroyed on driver-unbind. To do this, we introduce a Devres resource into > shmem::Object that we use in order to ensure that we release any SGTable > mappings on driver-unbind. > > There's some other slightly unfortunate caveats of this: > > * Drivers don't have explicit control at the moment over when unmapping > happens (which is exactly the same as the C side atm, so it might not be > a problem). > * We can't just return `SGTableMap` to the user through an Arc to attempt > to fix the last caveat - because that implies the gem object would need > to hold a reference count to the scatterlist mapping, which just leaves > us with the same problem. > > Signed-off-by: Lyude Paul <[email protected]>
I really like how simplified `sg_table` has become! Reviewed-by: Alexandre Courbot <[email protected]> With the customary final nits below. <...> > + > + /// Creates (if necessary) and returns an immutable reference to a > scatter-gather table of DMA > + /// pages for this object. > + /// > + /// This will pin the object in memory. It is expected that `dev` should > be a pointer to the > + /// same [`device::Device`] which `self` belongs to, otherwise this > function will return > + /// `Err(EINVAL)`. > + pub fn sg_table<'a>( > + &'a self, > + dev: &'a device::Device<Bound>, > + ) -> Result<&'a scatterlist::SGTable> { > + if dev.as_raw() != self.dev().as_ref().as_raw() { > + return Err(EINVAL); > + } > + > + let sgt_res = 'out: { > + // Fast path: sgt_res is already initialized > + if let Some(sgt_res) = self.sgt_res.as_ref() { > + break 'out sgt_res; > + } > + > + // Slow path: Grab the lock and see if we need to initialize > sgt_res. > + let _guard = self.sgt_lock.lock(); > + > + // If someone initialized it while we were waiting, we can exit > early. > + if let Some(sgt_res) = self.sgt_res.as_ref() { > + break 'out sgt_res; > + } > + > + // If not, finish initializing and return. > + self.sgt_res > + .populate(Devres::new(dev, SGTableMap::new(self))?); Maybe add a comment explaining that `populate` cannot return `false`, as its invocation it protected by the mutex? This helps understanding that the following unsafe block is ok. > + > + // SAFETY: We just populated sgt_res above. > + unsafe { self.sgt_res.as_ref().unwrap_unchecked() } > + }; > + > + Ok(sgt_res.access(dev)?) > + } > } > > impl<T: DriverObject, C: DeviceContext> Deref for Object<T, C> { > @@ -474,6 +545,63 @@ impl<D, R, C, const SIZE: usize> IoKnownSize for VMap<D, > R, C, SIZE> > #[cfg(CONFIG_64BIT)] > impl_vmap_io_capable!(VMap, u64); > > +/// A reference to a GEM object that is known to have a mapped [`SGTable`]. > +/// > +/// This is used by the Rust bindings with [`Devres`] in order to ensure > that mappings for SGTables > +/// on GEM shmem objects are revoked on driver-unbind. > +/// > +/// # Invariants > +/// > +/// - `self.obj` always points to a valid GEM object. > +/// - This object is proof that `self.obj.owner.sgt` has an initialized and > valid > +/// [`scatterlist::SGTable`]. The SGTable is not in `owner.sgt` anymore. > +pub struct SGTableMap<T: DriverObject, C: DeviceContext> { > + obj: NonNull<Object<T, C>>, > +} > + > +impl<T: DriverObject, C: DeviceContext> Deref for SGTableMap<T, C> { > + type Target = scatterlist::SGTable; > + > + fn deref(&self) -> &Self::Target { > + // SAFETY: > + // - The NonNull is guaranteed to be valid via our type invariants. > + // - The sgt field is guaranteed to be initialized and valid via our > type invariants. > + unsafe { > scatterlist::SGTable::from_raw((*self.obj.as_ref().as_raw_shmem()).sgt) } > + } > +} > + > +impl<T: DriverObject, C: DeviceContext> Drop for SGTableMap<T, C> { > + fn drop(&mut self) { > + // SAFETY: `obj` is always valid via our type invariants > + let obj = unsafe { self.obj.as_ref() }; > + let _lock = DmaResvGuard::new(obj); > + > + // SAFETY: We acquired the lock needed for calling this function > above > + unsafe { > bindings::__drm_gem_shmem_free_sgt_locked(obj.as_raw_shmem()) }; > + } > +} > + > +impl<T: DriverObject, C: DeviceContext> SGTableMap<T, C> { > + fn new(obj: &Object<T, C>) -> impl Init<Self, Error> { > + // INVARIANT: > + // - We call drm_gem_shmem_get_pages_sgt_locked below and check > whether or not it s/drm_gem_shmem_get_pages_sgt_locked/drm_gem_shmem_get_pages_sgt.
