* [PATCH v16 1/6] rust: drm: gem/shmem: Add DmaResvGuard helper
2026-06-02 17:24 [PATCH v16 0/6] Rust bindings for gem shmem Lyude Paul
@ 2026-06-02 17:25 ` Lyude Paul
2026-06-04 2:29 ` Claude review: " Claude Code Review Bot
2026-06-02 17:25 ` [PATCH v16 2/6] rust: drm: gem: Add vmap functions to shmem bindings Lyude Paul
` (5 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: Lyude Paul @ 2026-06-02 17:25 UTC (permalink / raw)
To: dri-devel, rust-for-linux, nouveau
Cc: Alexandre Courbot, Gary Guo, Christian König, driver-core,
Miguel Ojeda, Maarten Lankhorst, Alice Ryhl, Simona Vetter,
linux-kernel, Sumit Semwal, linux-media, Rafael J . Wysocki,
Thomas Zimmermann, Maxime Ripard, David Airlie, Benno Lossin,
linaro-mm-sig, Danilo Krummrich, Mukesh Kumar Chaurasiya,
Asahi Lina, Daniel Almeida, Lyude Paul, Greg Kroah-Hartman
Just a temporary holdover to make locking/unlocking the dma_resv lock much
easier.
Signed-off-by: Lyude Paul <lyude@redhat.com>
Co-authored-by: Alexandre Courbot <acourbot@nvidia.com>
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
rust/kernel/drm/gem/shmem.rs | 31 ++++++++++++++++++++++++++++++-
1 file changed, 30 insertions(+), 1 deletion(-)
diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
index 35d7523e164ff..066a820b0bffc 100644
--- a/rust/kernel/drm/gem/shmem.rs
+++ b/rust/kernel/drm/gem/shmem.rs
@@ -27,7 +27,10 @@
Deref,
DerefMut, //
},
- ptr::NonNull,
+ ptr::{
+ self,
+ NonNull, //
+ },
};
use gem::{
BaseObjectPrivate,
@@ -224,3 +227,29 @@ impl<T: DriverObject> driver::AllocImpl for Object<T> {
dumb_map_offset: None,
};
}
+
+/// Private helper-type for holding the `dma_resv` object for a GEM shmem object.
+///
+/// When this is dropped, the `dma_resv` lock is dropped as well.
+///
+// TODO: This should be replace with a WwMutex equivalent once we have such bindings in the kernel.
+struct DmaResvGuard<'a, T: DriverObject>(&'a Object<T>);
+
+impl<'a, T: DriverObject> DmaResvGuard<'a, T> {
+ #[inline(always)]
+ #[expect(unused)]
+ fn new(obj: &'a Object<T>) -> Self {
+ // SAFETY: This lock is initialized throughout the lifetime of `object`.
+ unsafe { bindings::dma_resv_lock(obj.raw_dma_resv(), ptr::null_mut()) };
+
+ Self(obj)
+ }
+}
+
+impl<'a, T: DriverObject> Drop for DmaResvGuard<'a, T> {
+ #[inline(always)]
+ fn drop(&mut self) {
+ // SAFETY: We are releasing the lock grabbed during the creation of this object.
+ unsafe { bindings::dma_resv_unlock(self.0.raw_dma_resv()) };
+ }
+}
--
2.54.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Claude review: rust: drm: gem/shmem: Add DmaResvGuard helper
2026-06-02 17:25 ` [PATCH v16 1/6] rust: drm: gem/shmem: Add DmaResvGuard helper Lyude Paul
@ 2026-06-04 2:29 ` Claude Code Review Bot
0 siblings, 0 replies; 18+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 2:29 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
A simple RAII guard for the DMA reservation lock. Clean and correct.
The return value of `dma_resv_lock` is discarded:
```rust
unsafe { bindings::dma_resv_lock(obj.raw_dma_resv(), ptr::null_mut()) };
```
With a NULL `ww_acquire_ctx`, `dma_resv_lock` degenerates to `mutex_lock` and always returns 0, so ignoring the return value is safe in practice. The TODO comment acknowledging this should be replaced with proper WwMutex bindings is appropriate.
The `#[expect(unused)]` on `new()` is fine since patch 2 removes it immediately. The `#[inline(always)]` on both `new()` and `drop()` is reasonable for a thin lock wrapper.
No issues.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v16 2/6] rust: drm: gem: Add vmap functions to shmem bindings
2026-06-02 17:24 [PATCH v16 0/6] Rust bindings for gem shmem Lyude Paul
2026-06-02 17:25 ` [PATCH v16 1/6] rust: drm: gem/shmem: Add DmaResvGuard helper Lyude Paul
@ 2026-06-02 17:25 ` Lyude Paul
2026-06-04 2:29 ` Claude review: " Claude Code Review Bot
2026-06-02 17:25 ` [PATCH v16 3/6] rust: sync: Add SetOnce::reset() Lyude Paul
` (4 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: Lyude Paul @ 2026-06-02 17:25 UTC (permalink / raw)
To: dri-devel, rust-for-linux, nouveau
Cc: Alexandre Courbot, Gary Guo, Christian König, driver-core,
Miguel Ojeda, Maarten Lankhorst, Alice Ryhl, Simona Vetter,
linux-kernel, Sumit Semwal, linux-media, Rafael J . Wysocki,
Thomas Zimmermann, Maxime Ripard, David Airlie, Benno Lossin,
linaro-mm-sig, Danilo Krummrich, Mukesh Kumar Chaurasiya,
Asahi Lina, Daniel Almeida, Lyude Paul, Greg Kroah-Hartman
One of the more obvious use cases for gem shmem objects is the ability to
create mappings into their contents. So, let's hook this up in our rust
bindings.
Signed-off-by: Lyude Paul <lyude@redhat.com>
---
V7:
* Switch over to the new iosys map bindings that use the Io trait
V8:
* Get rid of iosys_map bindings for now, only support non-iomem types
* s/as_shmem()/as_raw_shmem()
V9:
* Get rid of some outdated comments I missed
* Add missing SIZE check to raw_vmap()
* Add a proper unit test that ensures that we actually validate SIZE at
compile-time.
Turns out it takes only 34 lines to make a boilerplate DRM driver for a
kunit test :)
* Add unit tests
* Add some missing #[inline]s
V10:
* Correct issue with iomem error path
We previously called raw_vunmap() if we got an iomem allocation, but
raw_vunmap() was written such that it assumed all allocations were sysmem
allocations. Fix this by just making raw_vunmap() accept a iosys_map.
V11:
* Use Alexandre's clever solution to remove the macros we were using for
maintaining two different VMap types.
* Change the order of items in Object<T> to ensure that sgt_res is always
dropped before obj.
* Fix typo in Object.raw_vmap()
* s/raw_vmap()/make_vmap()/
Deduplicate code a bit more as well by using more generics here
V15:
* Add these patches back
* We only have one VMap type now!
* Use ObjectConfig::default() in unit tests since we unbroke it.
V16:
* Fix huge rebase error I made and did not notice that squashed 1.5 patches
together that were definitely not supposed to be squashed
* Update old commit message
rust/kernel/drm/gem/shmem.rs | 303 ++++++++++++++++++++++++++++++++++-
1 file changed, 302 insertions(+), 1 deletion(-)
diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
index 066a820b0bffc..843869056b1d7 100644
--- a/rust/kernel/drm/gem/shmem.rs
+++ b/rust/kernel/drm/gem/shmem.rs
@@ -18,11 +18,18 @@
Device, //
},
error::to_result,
+ io::{
+ Io,
+ IoCapable,
+ IoKnownSize, //
+ },
prelude::*,
sync::aref::ARef,
types::Opaque, //
};
use core::{
+ ffi::c_void,
+ mem::MaybeUninit, //
ops::{
Deref,
DerefMut, //
@@ -33,6 +40,7 @@
},
};
use gem::{
+ BaseObject,
BaseObjectPrivate,
DriverObject,
IntoGEMObject, //
@@ -177,6 +185,80 @@ extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {
// SAFETY: We're recovering the Kbox<> we created in gem_create_object()
let _ = unsafe { KBox::from_raw(this) };
}
+
+ /// Attempt to create a vmap from the gem object, and confirm the size of said vmap.
+ fn make_vmap<'a, R, const SIZE: usize>(&'a self) -> Result<VMap<T, R, SIZE>>
+ where
+ R: Deref<Target = Self> + From<&'a Self>,
+ {
+ // INVARIANT: We check here that the gem object is at least as large as `SIZE`.
+ if self.size() < SIZE {
+ return Err(ENOSPC);
+ }
+
+ let mut map: MaybeUninit<bindings::iosys_map> = MaybeUninit::uninit();
+ let guard = DmaResvGuard::new(self);
+
+ // SAFETY: drm_gem_shmem_vmap can be called with the DMA reservation lock held
+ to_result(unsafe {
+ bindings::drm_gem_shmem_vmap_locked(self.as_raw_shmem(), map.as_mut_ptr())
+ })?;
+
+ // Drop the guard explicitly here, since we may need to call raw_vunmap() (which re-acquires
+ // the lock).
+ drop(guard);
+
+ // SAFETY: The call to drm_gem_shmem_vmap_locked succeeded above, so we are guaranteed that
+ // map is properly initialized.
+ let map = unsafe { map.assume_init() };
+
+ // XXX: We don't currently support iomem allocations
+ if map.is_iomem {
+ // SAFETY:
+ // - The vmap operation above succeeded, guaranteeing that `map` points to a valid
+ // memory mapping.
+ // - We checked that this is an iomem allocation, making it safe to read vaddr_iomem
+ unsafe { self.raw_vunmap(map) };
+
+ Err(ENOTSUPP)
+ } else {
+ Ok(VMap {
+ // SAFETY: We checked that this is not an iomem allocation, making it safe to read
+ // vaddr
+ addr: unsafe { map.__bindgen_anon_1.vaddr },
+ owner: self.into(),
+ })
+ }
+ }
+
+ /// Unmap a vmap from the gem object.
+ ///
+ /// # Safety
+ ///
+ /// - The caller promises that `map` is a valid vmap on this gem object.
+ /// - The caller promises that the memory pointed to by map will no longer be accesed through
+ /// this instance.
+ unsafe fn raw_vunmap(&self, mut map: bindings::iosys_map) {
+ let _guard = DmaResvGuard::new(self);
+
+ // SAFETY:
+ // - This function is safe to call with the DMA reservation lock held.
+ // - Our `ARef` is proof that the underlying gem object here is initialized and thus safe to
+ // dereference.
+ unsafe { bindings::drm_gem_shmem_vunmap_locked(self.as_raw_shmem(), &mut map) };
+ }
+
+ /// Creates and returns a virtual kernel memory mapping for this object.
+ #[inline]
+ pub fn vmap<const SIZE: usize>(&self) -> Result<VMapRef<'_, T, SIZE>> {
+ self.make_vmap()
+ }
+
+ /// Creates and returns an owned reference to a virtual kernel memory mapping for this object.
+ #[inline]
+ pub fn owned_vmap<const SIZE: usize>(&self) -> Result<VMapOwned<T, SIZE>> {
+ self.make_vmap()
+ }
}
impl<T: DriverObject> Deref for Object<T> {
@@ -237,7 +319,6 @@ impl<T: DriverObject> driver::AllocImpl for Object<T> {
impl<'a, T: DriverObject> DmaResvGuard<'a, T> {
#[inline(always)]
- #[expect(unused)]
fn new(obj: &'a Object<T>) -> Self {
// SAFETY: This lock is initialized throughout the lifetime of `object`.
unsafe { bindings::dma_resv_lock(obj.raw_dma_resv(), ptr::null_mut()) };
@@ -253,3 +334,223 @@ fn drop(&mut self) {
unsafe { bindings::dma_resv_unlock(self.0.raw_dma_resv()) };
}
}
+
+macro_rules! impl_vmap_io_capable {
+ ($impl:ident, $ty:ty) => {
+ impl<D, R, const SIZE: usize> IoCapable<$ty> for $impl<D, R, SIZE>
+ where
+ D: DriverObject,
+ R: Deref<Target = Object<D>>,
+ {
+ #[inline(always)]
+ unsafe fn io_read(&self, address: usize) -> $ty {
+ let ptr = address as *mut $ty;
+
+ // SAFETY: The safety contract of `io_read` guarantees that address is a valid
+ // address within the bounds of `Self` of at least the size of $ty, and is properly
+ // aligned.
+ unsafe { ptr::read(ptr) }
+ }
+
+ #[inline(always)]
+ unsafe fn io_write(&self, value: $ty, address: usize) {
+ let ptr = address as *mut $ty;
+
+ // SAFETY: The safety contract of `io_write` guarantees that address is a valid
+ // address within the bounds of `Self` of at least the size of $ty, and is properly
+ // aligned.
+ unsafe { ptr::write(ptr, value) }
+ }
+ }
+ };
+}
+
+/// A reference to a virtual mapping for an shmem-based GEM object in kernel address space.
+///
+/// # Invariants
+///
+/// - The size of `owner` is >= SIZE.
+/// - The memory pointed to by addr remains valid at least until this object is dropped.
+pub struct VMap<D, R, const SIZE: usize = 0>
+where
+ D: DriverObject,
+ R: Deref<Target = Object<D>>,
+{
+ addr: *mut c_void,
+ owner: R,
+}
+
+/// An alias type for a reference to a shmem-based GEM object's VMap.
+pub type VMapRef<'a, D, const SIZE: usize = 0> = VMap<D, &'a Object<D>, SIZE>;
+
+/// An alias type for an owned reference to a shmem-based GEM object's VMap.
+pub type VMapOwned<D, const SIZE: usize = 0> = VMap<D, ARef<Object<D>>, SIZE>;
+
+impl<D, R, const SIZE: usize> VMap<D, R, SIZE>
+where
+ D: DriverObject,
+ R: Deref<Target = Object<D>>,
+{
+ /// Borrows a reference to the object that owns this virtual mapping.
+ #[inline(always)]
+ pub fn owner(&self) -> &Object<D> {
+ &self.owner
+ }
+}
+
+impl<D, R, const SIZE: usize> Drop for VMap<D, R, SIZE>
+where
+ D: DriverObject,
+ R: Deref<Target = Object<D>>,
+{
+ #[inline(always)]
+ fn drop(&mut self) {
+ // SAFETY:
+ // - Our existence is proof that this map was previously created using self.owner.
+ // - Since we are in Drop, we are guaranteed that no one will access the memory
+ // through this mapping after calling this.
+ unsafe {
+ self.owner.raw_vunmap(bindings::iosys_map {
+ is_iomem: false,
+ __bindgen_anon_1: bindings::iosys_map__bindgen_ty_1 { vaddr: self.addr },
+ })
+ };
+ }
+}
+
+impl<D, R, const SIZE: usize> Io for VMap<D, R, SIZE>
+where
+ D: DriverObject,
+ R: Deref<Target = Object<D>>,
+{
+ #[inline(always)]
+ fn addr(&self) -> usize {
+ self.addr as usize
+ }
+
+ #[inline(always)]
+ fn maxsize(&self) -> usize {
+ self.owner.size()
+ }
+}
+
+impl<D, R, const SIZE: usize> IoKnownSize for VMap<D, R, SIZE>
+where
+ D: DriverObject,
+ R: Deref<Target = Object<D>>,
+{
+ const MIN_SIZE: usize = SIZE;
+}
+
+impl_vmap_io_capable!(VMap, u8);
+impl_vmap_io_capable!(VMap, u16);
+impl_vmap_io_capable!(VMap, u32);
+#[cfg(CONFIG_64BIT)]
+impl_vmap_io_capable!(VMap, u64);
+
+#[kunit_tests(rust_drm_gem_shmem)]
+mod tests {
+ use super::*;
+ use crate::{
+ drm,
+ faux,
+ page::PAGE_SIZE, //
+ };
+
+ // The bare minimum needed to create a fake drm driver for kunit
+
+ #[pin_data]
+ struct KunitData {}
+ struct KunitDriver;
+ struct KunitFile;
+ #[pin_data]
+ struct KunitObject {}
+
+ const INFO: drm::DriverInfo = drm::DriverInfo {
+ major: 0,
+ minor: 0,
+ patchlevel: 0,
+ name: c"kunit",
+ desc: c"Kunit",
+ };
+
+ impl drm::file::DriverFile for KunitFile {
+ type Driver = KunitDriver;
+
+ fn open(_dev: &drm::Device<KunitDriver>) -> Result<Pin<KBox<Self>>> {
+ Ok(KBox::new(Self, GFP_KERNEL)?.into())
+ }
+ }
+
+ impl gem::DriverObject for KunitObject {
+ type Driver = KunitDriver;
+ type Args = ();
+
+ fn new(
+ _dev: &drm::Device<KunitDriver>,
+ _size: usize,
+ _args: Self::Args,
+ ) -> impl PinInit<Self, Error> {
+ try_pin_init!(KunitObject {})
+ }
+ }
+
+ #[vtable]
+ impl drm::Driver for KunitDriver {
+ type Data = KunitData;
+ type File = KunitFile;
+ type Object = Object<KunitObject>;
+
+ const INFO: drm::DriverInfo = INFO;
+ const IOCTLS: &'static [drm::ioctl::DrmIoctlDescriptor] = &[];
+ }
+
+ fn create_drm_dev() -> Result<(faux::Registration, ARef<drm::Device<KunitDriver>>)> {
+ // Create a faux DRM device so we can test gem object creation.
+ let data = try_pin_init!(KunitData {});
+ let dev = faux::Registration::new(c"Kunit", None)?;
+ let drm = drm::Device::<KunitDriver>::new(dev.as_ref(), data)?;
+
+ Ok((dev, drm))
+ }
+
+ #[test]
+ fn compile_time_vmap_sizes() -> Result {
+ let (_dev, drm) = create_drm_dev()?;
+
+ let obj = Object::<KunitObject>::new(&drm, PAGE_SIZE, ObjectConfig::default(), ())?;
+
+ // Try creating a normal vmap
+ obj.vmap::<PAGE_SIZE>()?;
+
+ // Try creating a vmap that's smaller then the size we specified
+ obj.vmap::<{ PAGE_SIZE - 100 }>()?;
+
+ // Make sure creating a vmap that's too large fails
+ assert!(obj.vmap::<{ PAGE_SIZE + 200 }>().is_err());
+
+ Ok(())
+ }
+
+ #[test]
+ fn vmap_io() -> Result {
+ let (_dev, drm) = create_drm_dev()?;
+
+ let obj = Object::<KunitObject>::new(&drm, PAGE_SIZE, ObjectConfig::default(), ())?;
+
+ let vmap = obj.vmap::<PAGE_SIZE>()?;
+
+ vmap.write8(0xDE, 0x0);
+ assert_eq!(vmap.read8(0x0), 0xDE);
+ vmap.write32(0xFFFFFFFF, 0x20);
+
+ assert_eq!(vmap.read32(0x20), 0xFFFFFFFF);
+
+ assert_eq!(vmap.read8(0x20), 0xFF);
+ assert_eq!(vmap.read8(0x21), 0xFF);
+ assert_eq!(vmap.read8(0x22), 0xFF);
+ assert_eq!(vmap.read8(0x23), 0xFF);
+
+ Ok(())
+ }
+}
--
2.54.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Claude review: rust: drm: gem: Add vmap functions to shmem bindings
2026-06-02 17:25 ` [PATCH v16 2/6] rust: drm: gem: Add vmap functions to shmem bindings Lyude Paul
@ 2026-06-04 2:29 ` Claude Code Review Bot
0 siblings, 0 replies; 18+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 2:29 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
This is the largest patch and adds the core vmap functionality. The design is well thought out.
**`make_vmap` guard lifetime**: The explicit `drop(guard)` before the iomem error path is correct — `raw_vunmap` re-acquires the lock internally:
```rust
let guard = DmaResvGuard::new(self);
to_result(unsafe {
bindings::drm_gem_shmem_vmap_locked(self.as_raw_shmem(), map.as_mut_ptr())
})?;
drop(guard);
```
Good defensive pattern.
**VMap::Drop reconstruction**: The `drop` reconstructs an `iosys_map` to pass to `raw_vunmap`:
```rust
self.owner.raw_vunmap(bindings::iosys_map {
is_iomem: false,
__bindgen_anon_1: bindings::iosys_map__bindgen_ty_1 { vaddr: self.addr },
})
```
This works because `make_vmap` already validated `!is_iomem` before constructing the `VMap`. The invariant is maintained.
**IoCapable implementations**: The `impl_vmap_io_capable!` macro implements `ptr::read`/`ptr::write` through raw pointer casts:
```rust
unsafe fn io_read(&self, address: usize) -> $ty {
let ptr = address as *mut $ty;
unsafe { ptr::read(ptr) }
}
```
The safety is delegated to the `Io` trait's `io_read`/`io_write` contract (alignment, bounds). This is correct.
**Minor**: The `VMap` struct's `addr` field is `*mut c_void` — this raw pointer doesn't implement `Send`/`Sync`, but `VMap` doesn't explicitly implement them either. Since the GEM object backing the mapping is thread-safe, and the mapping itself is valid for the lifetime of the owner, this might need explicit `Send`/`Sync` impls if callers need to share the mapping across threads. However, this may be intentionally restrictive for now.
**Tests**: Good coverage — `compile_time_vmap_sizes` validates the size check and `vmap_io` tests read/write through the mapping. The byte-level verification of a `write32` is a nice touch.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v16 3/6] rust: sync: Add SetOnce::reset()
2026-06-02 17:24 [PATCH v16 0/6] Rust bindings for gem shmem Lyude Paul
2026-06-02 17:25 ` [PATCH v16 1/6] rust: drm: gem/shmem: Add DmaResvGuard helper Lyude Paul
2026-06-02 17:25 ` [PATCH v16 2/6] rust: drm: gem: Add vmap functions to shmem bindings Lyude Paul
@ 2026-06-02 17:25 ` Lyude Paul
2026-06-04 2:29 ` Claude review: " Claude Code Review Bot
2026-06-02 17:25 ` [PATCH v16 4/6] rust: gem: shmem: Fix Default implementation for ObjectConfig Lyude Paul
` (3 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: Lyude Paul @ 2026-06-02 17:25 UTC (permalink / raw)
To: dri-devel, rust-for-linux, nouveau
Cc: Alexandre Courbot, Gary Guo, Christian König, driver-core,
Miguel Ojeda, Maarten Lankhorst, Alice Ryhl, Simona Vetter,
linux-kernel, Sumit Semwal, linux-media, Rafael J . Wysocki,
Thomas Zimmermann, Maxime Ripard, David Airlie, Benno Lossin,
linaro-mm-sig, Danilo Krummrich, Mukesh Kumar Chaurasiya,
Asahi Lina, Daniel Almeida, Lyude Paul, Greg Kroah-Hartman
This function simply drops the contents of the SetOnce, given a mutable
reference - since that proves we have exclusive access to the SetOnce.
Additionally, update the invariants for SetOnce to make it clear as to why
this is safe.
Signed-off-by: Lyude Paul <lyude@redhat.com>
---
rust/kernel/sync/set_once.rs | 60 +++++++++++++++++++++++++++++++-----
1 file changed, 52 insertions(+), 8 deletions(-)
diff --git a/rust/kernel/sync/set_once.rs b/rust/kernel/sync/set_once.rs
index 139cef05e935f..d6e4fc2695673 100644
--- a/rust/kernel/sync/set_once.rs
+++ b/rust/kernel/sync/set_once.rs
@@ -15,7 +15,7 @@
///
/// # Invariants
///
-/// - `init` may only increase in value.
+/// - `init` may only increase in value, unless modified through a mutable reference.
/// - `init` may only assume values in the range `0..=2`.
/// - `init == 0` if and only if `value` is uninitialized.
/// - `init == 1` if and only if there is exactly one thread with exclusive
@@ -110,17 +110,61 @@ pub fn copy(&self) -> Option<T>
{
self.as_ref().copied()
}
+
+ /// # Safety
+ ///
+ /// If this function returns `true`, `self` must be freed or `init` must be reset to `0`.
+ unsafe fn drop_val(&mut self) -> bool {
+ if *self.init.get_mut() != 2 {
+ return false;
+ }
+
+ let value = self.value.get_mut();
+ // SAFETY: Via our type invariants, `init` == 2 means `value` is initialized.
+ unsafe { value.assume_init_drop() };
+
+ true
+ }
+
+ /// Unset the [`SetOnce`].
+ ///
+ /// After this function is called, the [`SetOnce`] is empty and uninitialized. This function is
+ /// mainly intended for usage in destructors.
+ ///
+ /// Returns `true` if `self` was previously initialized.
+ ///
+ /// # Example
+ ///
+ /// ```
+ /// # use kernel::sync::SetOnce;
+ /// let mut value = SetOnce::new();
+ /// assert_eq!(value.populate(67), true);
+ ///
+ /// assert_eq!(value.reset(), true);
+ /// assert!(value.as_ref().is_none());
+ /// assert_eq!(value.reset(), false);
+ ///
+ /// assert_eq!(value.populate(69), true);
+ /// ```
+ pub fn reset(&mut self) -> bool {
+ // SAFETY: We write `0` to init below if this returns true.
+ let dropped = unsafe { self.drop_val() };
+ if dropped {
+ // INVARIANT:
+ // - We set `init` back to 0 through a mutable reference.
+ // - We dropped `value` above.
+ *self.init.get_mut() = 0;
+ }
+
+ dropped
+ }
}
impl<T> Drop for SetOnce<T> {
+ #[inline(always)]
fn drop(&mut self) {
- if *self.init.get_mut() == 2 {
- let value = self.value.get_mut();
- // SAFETY: By the type invariants of `Self`, `self.init == 2` means that `self.value`
- // contains a valid value. We have exclusive access, as we hold a `mut` reference to
- // `self`.
- unsafe { value.assume_init_drop() };
- }
+ // SAFETY: We are dropping this value.
+ unsafe { self.drop_val() };
}
}
--
2.54.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Claude review: rust: sync: Add SetOnce::reset()
2026-06-02 17:25 ` [PATCH v16 3/6] rust: sync: Add SetOnce::reset() Lyude Paul
@ 2026-06-04 2:29 ` Claude Code Review Bot
0 siblings, 0 replies; 18+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 2:29 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
Adds `reset()` which takes `&mut self` (proving exclusive access), drops the value, and resets `init` to 0.
**Invariant update**: The change from "init may only increase in value" to "init may only increase in value, unless modified through a mutable reference" is correct — `&mut self` guarantees no concurrent observers.
**`drop_val` safety contract**: The `unsafe fn drop_val` has a critical safety requirement:
```rust
/// If this function returns `true`, `self` must be freed or `init` must be reset to `0`.
```
Between `drop_val()` returning `true` and the caller resetting `init`, there's a window where `init == 2` but the value is dropped. This is safe because `&mut self` ensures exclusive access, but the contract is correctly documented. Both callers (`reset` and `Drop`) satisfy it.
**Doctest**: The example is good and demonstrates the full reset/repopulate cycle:
```rust
assert_eq!(value.populate(67), true);
assert_eq!(value.reset(), true);
assert!(value.as_ref().is_none());
assert_eq!(value.reset(), false);
assert_eq!(value.populate(69), true);
```
No issues.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v16 4/6] rust: gem: shmem: Fix Default implementation for ObjectConfig
2026-06-02 17:24 [PATCH v16 0/6] Rust bindings for gem shmem Lyude Paul
` (2 preceding siblings ...)
2026-06-02 17:25 ` [PATCH v16 3/6] rust: sync: Add SetOnce::reset() Lyude Paul
@ 2026-06-02 17:25 ` Lyude Paul
2026-06-04 2:29 ` Claude review: " Claude Code Review Bot
2026-06-02 17:25 ` [PATCH v16 5/6] rust: faux: Allow retrieving a bound Device Lyude Paul
` (2 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: Lyude Paul @ 2026-06-02 17:25 UTC (permalink / raw)
To: dri-devel, rust-for-linux, nouveau
Cc: Alexandre Courbot, Gary Guo, Christian König, driver-core,
Miguel Ojeda, Maarten Lankhorst, Alice Ryhl, Simona Vetter,
linux-kernel, Sumit Semwal, linux-media, Rafael J . Wysocki,
Thomas Zimmermann, Maxime Ripard, David Airlie, Benno Lossin,
linaro-mm-sig, Danilo Krummrich, Mukesh Kumar Chaurasiya,
Asahi Lina, Daniel Almeida, Lyude Paul, Greg Kroah-Hartman
I completely forgot when coming up with this type that #[derive(Default)]
only works if all generics mentioned in the type implement Default (and T
usually doesn't). This being said: We don't use `T` for anything besides
using it for a reference type, so whether or not it implements `Default`
shouldn't actually need to matter.
So, fix this by just manually implementing Default instead of deriving it.
Signed-off-by: Lyude Paul <lyude@redhat.com>
---
rust/kernel/drm/gem/shmem.rs | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
index 843869056b1d7..d8bda8c8e2fb2 100644
--- a/rust/kernel/drm/gem/shmem.rs
+++ b/rust/kernel/drm/gem/shmem.rs
@@ -50,7 +50,6 @@
///
/// This is used with [`Object::new()`] to control various properties that can only be set when
/// initially creating a shmem-backed GEM object.
-#[derive(Default)]
pub struct ObjectConfig<'a, T: DriverObject> {
/// Whether to set the write-combine map flag.
pub map_wc: bool,
@@ -61,6 +60,16 @@ pub struct ObjectConfig<'a, T: DriverObject> {
pub parent_resv_obj: Option<&'a Object<T>>,
}
+impl<'a, T: DriverObject> Default for ObjectConfig<'a, T> {
+ #[inline(always)]
+ fn default() -> Self {
+ Self {
+ map_wc: false,
+ parent_resv_obj: None,
+ }
+ }
+}
+
/// A shmem-backed GEM object.
///
/// # Invariants
--
2.54.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v16 5/6] rust: faux: Allow retrieving a bound Device
2026-06-02 17:24 [PATCH v16 0/6] Rust bindings for gem shmem Lyude Paul
` (3 preceding siblings ...)
2026-06-02 17:25 ` [PATCH v16 4/6] rust: gem: shmem: Fix Default implementation for ObjectConfig Lyude Paul
@ 2026-06-02 17:25 ` Lyude Paul
2026-06-04 2:29 ` Claude review: " Claude Code Review Bot
2026-06-02 17:25 ` [PATCH v16 6/6] rust: drm: gem: Introduce shmem::Object::sg_table() Lyude Paul
2026-06-04 2:29 ` Claude review: Rust bindings for gem shmem Claude Code Review Bot
6 siblings, 1 reply; 18+ messages in thread
From: Lyude Paul @ 2026-06-02 17:25 UTC (permalink / raw)
To: dri-devel, rust-for-linux, nouveau
Cc: Alexandre Courbot, Gary Guo, Christian König, driver-core,
Miguel Ojeda, Maarten Lankhorst, Alice Ryhl, Simona Vetter,
linux-kernel, Sumit Semwal, linux-media, Rafael J . Wysocki,
Thomas Zimmermann, Maxime Ripard, David Airlie, Benno Lossin,
linaro-mm-sig, Danilo Krummrich, Mukesh Kumar Chaurasiya,
Asahi Lina, Daniel Almeida, Lyude Paul, Greg Kroah-Hartman
When writing up some rust code that used faux devices for unit testing, I
noticed that we never actually added the Bound device context to
faux::Registration's AsRef<device::Device> implementation. This being said:
the Registration object itself is proof that a driver is bound to the
device - so this should be safe.
Signed-off-by: Lyude Paul <lyude@redhat.com>
---
rust/kernel/faux.rs | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/rust/kernel/faux.rs b/rust/kernel/faux.rs
index 43b4974f48cd2..e0856b2964a2c 100644
--- a/rust/kernel/faux.rs
+++ b/rust/kernel/faux.rs
@@ -25,7 +25,8 @@
///
/// # Invariants
///
-/// `self.0` always holds a valid pointer to an initialized and registered [`struct faux_device`].
+/// - `self.0` always holds a valid pointer to an initialized and registered [`struct faux_device`].
+/// - This object is proof that the object described by this `Registration` is bound to a device.
///
/// [`struct faux_device`]: srctree/include/linux/device/faux.h
pub struct Registration(NonNull<bindings::faux_device>);
@@ -59,8 +60,8 @@ fn as_raw(&self) -> *mut bindings::faux_device {
}
}
-impl AsRef<device::Device> for Registration {
- fn as_ref(&self) -> &device::Device {
+impl AsRef<device::Device<device::Bound>> for Registration {
+ fn as_ref(&self) -> &device::Device<device::Bound> {
// SAFETY: The underlying `device` in `faux_device` is guaranteed by the C API to be
// a valid initialized `device`.
unsafe { device::Device::from_raw(addr_of_mut!((*self.as_raw()).dev)) }
--
2.54.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Claude review: rust: faux: Allow retrieving a bound Device
2026-06-02 17:25 ` [PATCH v16 5/6] rust: faux: Allow retrieving a bound Device Lyude Paul
@ 2026-06-04 2:29 ` Claude Code Review Bot
0 siblings, 0 replies; 18+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 2:29 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
Changes `AsRef<device::Device>` to `AsRef<device::Device<device::Bound>>`:
```rust
-impl AsRef<device::Device> for Registration {
- fn as_ref(&self) -> &device::Device {
+impl AsRef<device::Device<device::Bound>> for Registration {
+ fn as_ref(&self) -> &device::Device<device::Bound> {
```
The justification is correct — a `Registration` is proof of device binding, so returning `Device<Bound>` is sound. The added invariant bullet point documents this:
```
+/// - This object is proof that the object described by this `Registration` is bound to a device.
```
**Potential concern**: This is a breaking API change — any existing code calling `registration.as_ref()` expecting `&device::Device` (without `Bound`) will break. Since this is in the kernel tree and patch 6 is the consumer, this is fine, but downstream users of the faux crate should be aware.
No issues for this series.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v16 6/6] rust: drm: gem: Introduce shmem::Object::sg_table()
2026-06-02 17:24 [PATCH v16 0/6] Rust bindings for gem shmem Lyude Paul
` (4 preceding siblings ...)
2026-06-02 17:25 ` [PATCH v16 5/6] rust: faux: Allow retrieving a bound Device Lyude Paul
@ 2026-06-02 17:25 ` Lyude Paul
2026-06-04 2:29 ` Claude review: " Claude Code Review Bot
2026-06-04 2:29 ` Claude review: Rust bindings for gem shmem Claude Code Review Bot
6 siblings, 1 reply; 18+ messages in thread
From: Lyude Paul @ 2026-06-02 17:25 UTC (permalink / raw)
To: dri-devel, rust-for-linux, nouveau
Cc: Alexandre Courbot, Gary Guo, Christian König, driver-core,
Miguel Ojeda, Maarten Lankhorst, Alice Ryhl, Simona Vetter,
linux-kernel, Sumit Semwal, linux-media, Rafael J . Wysocki,
Thomas Zimmermann, Maxime Ripard, David Airlie, Benno Lossin,
linaro-mm-sig, Danilo Krummrich, Mukesh Kumar Chaurasiya,
Asahi Lina, Daniel Almeida, Lyude Paul, Greg Kroah-Hartman
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 <lyude@redhat.com>
---
V3:
* Rename OwnedSGTable to shmem::SGTable. Since the current version of the
SGTable abstractions now has a `Owned` and `Borrowed` variant, I think
renaming this to shmem::SGTable makes things less confusing.
We do however, keep the name of owned_sg_table() as-is.
V4:
* Clarify safety comments for SGTable to explain why the object is
thread-safe.
* Rename from SGTableRef to SGTable
V10:
* Use Devres in order to ensure that SGTables are revocable, and are
unmapped on driver-unbind.
V11:
* s/create_sg_table()/get_sg_table()
* Get rid of extraneous `ret = ` in shmem::Object::get_sg_table()
V12:
* Actually move sgt_res in this patch and not the next one
V13:
* Use DmaResvGuard suggestion from Alexander
* Use Alexander's (much better) solution for get_sg_table()
* Use SetOnce instead of UnsafeCell
* s/SGTableRef/SGTableMap
* Fix typo in SGTableMap documentation
* Create fallible constructor for SGTableMap
* Don't reuse dma_resv lock for protecting Object contents, just use Mutex
+ SetOnce
* Drop use of drm_gem_shmem_get_pages_sgt_locked(), since we don't need to
hold the dma_resv lock ourselves for anything but this function.
* Check that the device we receive in the bounds for sg_table() and
owned_sg_table() that said Device is in fact, the correct device.
* Remove redundant docs in owned_sg_table(), just point it back to
sg_table().
* Implement Deborah's suggestion to fix double-free in
free_callback()
* Restore original order of Object<T>
* Fix doc typo for SGTableMap
V14:
* Use new InitOnce container over the Mutex/SetOnce horror show we had
before.
* Start using LazyInit container for storing Devres for sgt unmap
* Add some kunit tests for sg_table (not sure why I didn't do this before)
using some of the boilerplate code leftover from the vmap bindings
* Get rid of the owned SGTable variant for now, we'll add it back in a
future patch if people actually need it.
* Use new LazyInit container from me to get rid of the horrid
Mutex<SetOnce<>> mess.
* Add the best we can do for unit tests w/r/t SGTable at the moment
V16:
* Get rid of LazyInit, go back to SetOnce, use trick that Alice recommended
that is a lot cleaner.
* Fix horrid rebasing mistake
rust/kernel/drm/gem/shmem.rs | 164 +++++++++++++++++++++++++++++++++--
1 file changed, 155 insertions(+), 9 deletions(-)
diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
index d8bda8c8e2fb2..5f27e8d59e765 100644
--- a/rust/kernel/drm/gem/shmem.rs
+++ b/rust/kernel/drm/gem/shmem.rs
@@ -11,20 +11,34 @@
use crate::{
container_of,
+ device::{
+ self,
+ Bound, //
+ },
+ devres::*,
drm::{
driver,
gem,
private::Sealed,
Device, //
},
- error::to_result,
+ error::{
+ from_err_ptr,
+ to_result, //
+ },
io::{
Io,
IoCapable,
IoKnownSize, //
},
prelude::*,
- sync::aref::ARef,
+ scatterlist,
+ sync::{
+ aref::ARef,
+ new_mutex,
+ Mutex,
+ SetOnce, //
+ },
types::Opaque, //
};
use core::{
@@ -83,6 +97,11 @@ pub struct Object<T: DriverObject> {
obj: Opaque<bindings::drm_gem_shmem_object>,
/// Parent object that owns this object's DMA reservation object.
parent_resv_obj: Option<ARef<Object<T>>>,
+ /// Devres object for unmapping any SGTable on driver-unbind.
+ sgt_res: SetOnce<Devres<SGTableMap<T>>>,
+ #[pin]
+ /// Lock for protecting initialization of `sgt_res`.
+ sgt_lock: Mutex<()>,
#[pin]
inner: T,
}
@@ -135,6 +154,8 @@ pub fn new(
try_pin_init!(Self {
obj <- Opaque::init_zeroed(),
parent_resv_obj: config.parent_resv_obj.map(|p| p.into()),
+ sgt_res: SetOnce::new(),
+ sgt_lock <- new_mutex!(()),
inner <- T::new(dev, size, args),
}),
GFP_KERNEL,
@@ -178,18 +199,23 @@ extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {
// - DRM always passes a valid gem object here
// - We used drm_gem_shmem_create() in our create_gem_object callback, so we know that
// `obj` is contained within a drm_gem_shmem_object
- let this = unsafe { container_of!(obj, bindings::drm_gem_shmem_object, base) };
-
- // SAFETY:
- // - We're in free_callback - so this function is safe to call.
- // - We won't be using the gem resources on `this` after this call.
- unsafe { bindings::drm_gem_shmem_release(this) };
+ let base = unsafe { container_of!(obj, bindings::drm_gem_shmem_object, base) };
// SAFETY:
// - We verified above that `obj` is valid, which makes `this` valid
// - This function is set in AllocOps, so we know that `this` is contained within a
// `Object<T>`
- let this = unsafe { container_of!(Opaque::cast_from(this), Self, obj) }.cast_mut();
+ let this = unsafe { container_of!(Opaque::cast_from(base), Self, obj) }.cast_mut();
+
+ // We need to drop `sgt_res` first, since doing so requires that the GEM object is still
+ // alive.
+ // SAFETY: We verified above that `this` is valid.
+ unsafe { &mut (*this).sgt_res }.reset();
+
+ // SAFETY:
+ // - We're in free_callback - so this function is safe to call.
+ // - We won't be using the gem resources on `this` after this call.
+ unsafe { bindings::drm_gem_shmem_release(base) };
// SAFETY: We're recovering the Kbox<> we created in gem_create_object()
let _ = unsafe { KBox::from_raw(this) };
@@ -268,6 +294,45 @@ pub fn vmap<const SIZE: usize>(&self) -> Result<VMapRef<'_, T, SIZE>> {
pub fn owned_vmap<const SIZE: usize>(&self) -> Result<VMapOwned<T, SIZE>> {
self.make_vmap()
}
+
+ /// 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))?);
+
+ // SAFETY: We just populated sgt_res above.
+ unsafe { self.sgt_res.as_ref().unwrap_unchecked() }
+ };
+
+ Ok(sgt_res.access(dev)?)
+ }
}
impl<T: DriverObject> Deref for Object<T> {
@@ -457,6 +522,63 @@ impl<D, R, const SIZE: usize> IoKnownSize for VMap<D, R, 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`].
+pub struct SGTableMap<T: DriverObject> {
+ obj: NonNull<Object<T>>,
+}
+
+impl<T: DriverObject> Deref for SGTableMap<T> {
+ 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> Drop for SGTableMap<T> {
+ 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> SGTableMap<T> {
+ fn new(obj: &Object<T>) -> impl Init<Self, Error> {
+ // INVARIANT:
+ // - We call drm_gem_shmem_get_pages_sgt_locked below and check whether or not it
+ // succeeds, fulfilling the invariant of SGTableMap that the object's `sgt` field is
+ // initialized.
+ // SAFETY:
+ // - `obj` is fully initialized, making this function safe to call.
+ from_err_ptr(unsafe { bindings::drm_gem_shmem_get_pages_sgt(obj.as_raw_shmem()) })?;
+
+ Ok(Self { obj: obj.into() })
+ }
+}
+
+// SAFETY: The NonNull in SGTableMap is guaranteed valid by our type invariants, and the GEM object
+// it points to is guaranteed to be thread-safe.
+unsafe impl<T: DriverObject> Send for SGTableMap<T> {}
+// SAFETY: The NonNull in SGTableMap is guaranteed valid by our type invariants, and the GEM object
+// it points to is guaranteed to be thread-safe.
+unsafe impl<T: DriverObject> Sync for SGTableMap<T> {}
+
#[kunit_tests(rust_drm_gem_shmem)]
mod tests {
use super::*;
@@ -562,4 +684,28 @@ fn vmap_io() -> Result {
Ok(())
}
+
+ // TODO: I would love to actually test the success paths of sg_table(), but that would require
+ // also implementing dummy dma_ops so that trying to create a mapping doesn't explode. So, leave
+ // that for someone else.
+
+ // Ensures that passing the wrong device to sg_table() fails as we expect, and also ensure it
+ // skips initializing `sgt_res` since we could otherwise create `sgt_res` with the wrong device
+ // bound to it.
+ #[test]
+ fn fail_sg_table_on_wrong_dev() -> Result {
+ let (_dev, drm) = create_drm_dev()?;
+ let wrong_dev = faux::Registration::new(c"EvilKunit", None)?;
+
+ let obj = Object::<KunitObject>::new(&drm, PAGE_SIZE, ObjectConfig::default(), ())?;
+
+ assert_eq!(obj.sg_table(wrong_dev.as_ref()).err().unwrap(), EINVAL);
+
+ // If sgt_res was not initialized mistakenly with the wrong device, this should still fail.
+ assert_eq!(obj.sg_table(wrong_dev.as_ref()).err().unwrap(), EINVAL);
+
+ // TODO: Someday, we should test that creating an sg_table here still succeeds.
+
+ Ok(())
+ }
}
--
2.54.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Claude review: rust: drm: gem: Introduce shmem::Object::sg_table()
2026-06-02 17:25 ` [PATCH v16 6/6] rust: drm: gem: Introduce shmem::Object::sg_table() Lyude Paul
@ 2026-06-04 2:29 ` Claude Code Review Bot
0 siblings, 0 replies; 18+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 2:29 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
The most complex patch. Several observations:
**Missing C-side function**: `SGTableMap::drop` calls:
```rust
unsafe { bindings::__drm_gem_shmem_free_sgt_locked(obj.as_raw_shmem()) };
```
This function does not exist in the current drm-next tree. It presumably lives in the "first half" of the series already pushed to drm-rust-next. If it's missing from the base, this won't compile. This should be verified against the actual base tree.
**Import restructuring**: The patch changes `device` from `crate::drm::device` to `crate::device`:
```rust
+ device::{
+ self,
+ Bound, //
+ },
drm::{
+ driver,
+ gem,
+ private::Sealed,
Device, //
},
```
This changes the meaning of `device::Device<T::Driver>` in the existing `dev()` method. On the actual drm-rust-next base, this may already be reconciled. Worth verifying.
**Double-checked locking in `sg_table()`**: The pattern is correct:
```rust
if let Some(sgt_res) = self.sgt_res.as_ref() { // fast path (Acquire)
break 'out sgt_res;
}
let _guard = self.sgt_lock.lock(); // slow path
if let Some(sgt_res) = self.sgt_res.as_ref() { // re-check
break 'out sgt_res;
}
self.sgt_res.populate(...); // Release
```
The `SetOnce` uses `Acquire` on read and `Release` on write, which provides the necessary happens-before guarantees for the double-checked locking pattern.
**SGTableMap lifetime safety**: `SGTableMap` holds `NonNull<Object<T>>` without an `ARef`:
```rust
pub struct SGTableMap<T: DriverObject> {
obj: NonNull<Object<T>>,
}
```
This is safe because `SGTableMap` is stored inside the `Object<T>` itself (via `sgt_res`), so the object outlives the mapping. The `free_callback` explicitly resets `sgt_res` before releasing the GEM object:
```rust
unsafe { &mut (*this).sgt_res }.reset(); // drops SGTableMap first
unsafe { bindings::drm_gem_shmem_release(base) }; // then releases GEM
let _ = unsafe { KBox::from_raw(this) }; // then frees memory
```
This ordering is correct and critical.
**Device identity check**:
```rust
if dev.as_raw() != self.dev().as_ref().as_raw() {
return Err(EINVAL);
}
```
Comparing raw device pointers to verify the caller passed the correct device is a good defensive check. The test `fail_sg_table_on_wrong_dev` verifies this works.
**`SGTableMap::new` discards the sg_table pointer**:
```rust
fn new(obj: &Object<T>) -> impl Init<Self, Error> {
from_err_ptr(unsafe { bindings::drm_gem_shmem_get_pages_sgt(obj.as_raw_shmem()) })?;
Ok(Self { obj: obj.into() })
}
```
The returned `sg_table*` is discarded. This is correct because `drm_gem_shmem_get_pages_sgt` stores the SGT internally in `shmem->sgt`, which is what `SGTableMap::deref` reads later.
**Minor style**: The `#[pin]` attribute appears before the doc comment on `sgt_lock`:
```rust
+ #[pin]
+ /// Lock for protecting initialization of `sgt_res`.
+ sgt_lock: Mutex<()>,
```
Convention is doc comments before attributes, though both orderings are valid Rust.
**Test coverage**: The `fail_sg_table_on_wrong_dev` test is good but limited — as the TODOs note, testing the success path requires mock DMA ops. The comment is honest about this limitation, which is appreciated.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 18+ messages in thread
* Claude review: Rust bindings for gem shmem
2026-06-02 17:24 [PATCH v16 0/6] Rust bindings for gem shmem Lyude Paul
` (5 preceding siblings ...)
2026-06-02 17:25 ` [PATCH v16 6/6] rust: drm: gem: Introduce shmem::Object::sg_table() Lyude Paul
@ 2026-06-04 2:29 ` Claude Code Review Bot
6 siblings, 0 replies; 18+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 2:29 UTC (permalink / raw)
To: dri-devel-reviews
Overall Series Review
Subject: Rust bindings for gem shmem
Author: Lyude Paul <lyude@redhat.com>
Patches: 7
Reviewed: 2026-06-04T12:29:53.038433
---
This is v16 of the Rust GEM shmem bindings series from Lyude Paul, building on work originally from Asahi Lina and Daniel Almeida. The series adds DMA reservation locking helpers, vmap support, `SetOnce::reset()`, a `Default` fix for `ObjectConfig`, faux device bound-context support, and scatter-gather table integration. The code is well-structured with clear safety documentation and thoughtful RAII patterns.
The overall architecture is sound: `DmaResvGuard` for scoped locking, `VMap` for typed memory mappings with compile-time size validation, and `SGTableMap` wrapped in `Devres` to ensure revocation on driver unbind. The double-checked locking pattern for lazy SGT initialization is correct with proper acquire/release ordering.
Key concerns:
1. Patch 6 references `__drm_gem_shmem_free_sgt_locked`, which does not exist in the current drm-next tree. This likely depends on a C-side export that should be in drm-rust-next but should be verified.
2. The import restructuring in patch 6 (changing `device` from `crate::drm::device` to `crate::device`) changes the meaning of the `dev()` return type. This may be correct on the intended base tree (drm-rust-next, base-commit b78dab829) but warrants attention.
3. Minor style and safety-comment nits throughout.
---
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v18 4/4] rust: drm: gem: Introduce shmem::Object::sg_table()
2026-06-04 19:24 [PATCH v18 0/4] " Lyude Paul
@ 2026-06-04 19:24 ` Lyude Paul
2026-06-04 20:03 ` Claude review: " Claude Code Review Bot
0 siblings, 1 reply; 18+ messages in thread
From: Lyude Paul @ 2026-06-04 19:24 UTC (permalink / raw)
To: dri-devel, rust-for-linux, nouveau
Cc: Alexandre Courbot, Gary Guo, Christian König, driver-core,
Miguel Ojeda, Maarten Lankhorst, Alice Ryhl, Simona Vetter,
linux-kernel, Sumit Semwal, linux-media, Rafael J . Wysocki,
Thomas Zimmermann, Maxime Ripard, David Airlie, Benno Lossin,
linaro-mm-sig, Danilo Krummrich, Mukesh Kumar Chaurasiya,
Asahi Lina, Daniel Almeida, Lyude Paul, Greg Kroah-Hartman
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 <lyude@redhat.com>
---
V3:
* Rename OwnedSGTable to shmem::SGTable. Since the current version of the
SGTable abstractions now has a `Owned` and `Borrowed` variant, I think
renaming this to shmem::SGTable makes things less confusing.
We do however, keep the name of owned_sg_table() as-is.
V4:
* Clarify safety comments for SGTable to explain why the object is
thread-safe.
* Rename from SGTableRef to SGTable
V10:
* Use Devres in order to ensure that SGTables are revocable, and are
unmapped on driver-unbind.
V11:
* s/create_sg_table()/get_sg_table()
* Get rid of extraneous `ret = ` in shmem::Object::get_sg_table()
V12:
* Actually move sgt_res in this patch and not the next one
V13:
* Use DmaResvGuard suggestion from Alexander
* Use Alexander's (much better) solution for get_sg_table()
* Use SetOnce instead of UnsafeCell
* s/SGTableRef/SGTableMap
* Fix typo in SGTableMap documentation
* Create fallible constructor for SGTableMap
* Don't reuse dma_resv lock for protecting Object contents, just use Mutex
+ SetOnce
* Drop use of drm_gem_shmem_get_pages_sgt_locked(), since we don't need to
hold the dma_resv lock ourselves for anything but this function.
* Check that the device we receive in the bounds for sg_table() and
owned_sg_table() that said Device is in fact, the correct device.
* Remove redundant docs in owned_sg_table(), just point it back to
sg_table().
* Implement Deborah's suggestion to fix double-free in
free_callback()
* Restore original order of Object<T>
* Fix doc typo for SGTableMap
V14:
* Use new InitOnce container over the Mutex/SetOnce horror show we had
before.
* Start using LazyInit container for storing Devres for sgt unmap
* Add some kunit tests for sg_table (not sure why I didn't do this before)
using some of the boilerplate code leftover from the vmap bindings
* Get rid of the owned SGTable variant for now, we'll add it back in a
future patch if people actually need it.
* Use new LazyInit container from me to get rid of the horrid
Mutex<SetOnce<>> mess.
* Add the best we can do for unit tests w/r/t SGTable at the moment
V16:
* Get rid of LazyInit, go back to SetOnce, use trick that Alice recommended
that is a lot cleaner.
* Fix horrid rebasing mistake
V17:
* Rebase
* Fix missing safety comment in free_callback() (we forgot to justify why
&mut is safe in `unsafe { &mut (*this).sgt_res }.reset()`)
V18:
* Use ManuallyDrop instead of SetOnce::reset()
rust/kernel/drm/gem/shmem.rs | 172 +++++++++++++++++++++++++++++++++--
1 file changed, 162 insertions(+), 10 deletions(-)
diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
index 1f05a5bc5fe66..a9970fca1d298 100644
--- a/rust/kernel/drm/gem/shmem.rs
+++ b/rust/kernel/drm/gem/shmem.rs
@@ -11,6 +11,11 @@
use crate::{
container_of,
+ device::{
+ self,
+ Bound, //
+ },
+ devres::*,
drm::{
driver,
gem,
@@ -19,20 +24,32 @@
DeviceContext,
Registered, //
},
- error::to_result,
+ error::{
+ from_err_ptr,
+ to_result, //
+ },
io::{
Io,
IoCapable,
IoKnownSize, //
},
prelude::*,
- sync::aref::ARef,
+ scatterlist,
+ sync::{
+ aref::ARef,
+ new_mutex,
+ Mutex,
+ SetOnce, //
+ },
types::Opaque, //
};
use core::{
ffi::c_void,
marker::PhantomData,
- mem::MaybeUninit, //
+ mem::{
+ ManuallyDrop,
+ MaybeUninit, //
+ },
ops::{
Deref,
DerefMut, //
@@ -87,6 +104,11 @@ pub struct Object<T: DriverObject, C: DeviceContext = Registered> {
obj: Opaque<bindings::drm_gem_shmem_object>,
/// Parent object that owns this object's DMA reservation object.
parent_resv_obj: Option<ARef<Object<T, C>>>,
+ /// Devres object for unmapping any SGTable on driver-unbind.
+ sgt_res: ManuallyDrop<SetOnce<Devres<SGTableMap<T, C>>>>,
+ #[pin]
+ /// Lock for protecting initialization of `sgt_res`.
+ sgt_lock: Mutex<()>,
#[pin]
inner: T,
_ctx: PhantomData<C>,
@@ -145,6 +167,8 @@ pub fn new(
try_pin_init!(Self {
obj <- Opaque::init_zeroed(),
parent_resv_obj: config.parent_resv_obj.map(|p| p.into()),
+ sgt_res: ManuallyDrop::new(SetOnce::new()),
+ sgt_lock <- new_mutex!(()),
inner <- T::new(dev, size, args),
_ctx: PhantomData::<C>,
}),
@@ -189,18 +213,26 @@ extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {
// - DRM always passes a valid gem object here
// - We used drm_gem_shmem_create() in our create_gem_object callback, so we know that
// `obj` is contained within a drm_gem_shmem_object
- let this = unsafe { container_of!(obj, bindings::drm_gem_shmem_object, base) };
-
- // SAFETY:
- // - We're in free_callback - so this function is safe to call.
- // - We won't be using the gem resources on `this` after this call.
- unsafe { bindings::drm_gem_shmem_release(this) };
+ let base = unsafe { container_of!(obj, bindings::drm_gem_shmem_object, base) };
// SAFETY:
// - We verified above that `obj` is valid, which makes `this` valid
// - This function is set in AllocOps, so we know that `this` is contained within a
// `Object<T, C>`
- let this = unsafe { container_of!(Opaque::cast_from(this), Self, obj) }.cast_mut();
+ let this = unsafe { container_of!(Opaque::cast_from(base), Self, obj) }.cast_mut();
+
+ // We need to drop `sgt_res` first, since doing so requires that the GEM object is still
+ // alive.
+ // SAFETY:
+ // - We verified above that `this` is valid.
+ // - We are in free_callback, guaranteeing we have exclusive access to `this` and that
+ // `sgt_res` will not be used after dropping it here.
+ unsafe { ManuallyDrop::drop(&mut (*this).sgt_res) };
+
+ // SAFETY:
+ // - We're in free_callback - so this function is safe to call.
+ // - We won't be using the gem resources on `this` after this call.
+ unsafe { bindings::drm_gem_shmem_release(base) };
// SAFETY: We're recovering the Kbox<> we created in gem_create_object()
let _ = unsafe { KBox::from_raw(this) };
@@ -279,6 +311,45 @@ pub fn vmap<const SIZE: usize>(&self) -> Result<VMapRef<'_, T, C, SIZE>> {
pub fn owned_vmap<const SIZE: usize>(&self) -> Result<VMapOwned<T, C, SIZE>> {
self.make_vmap()
}
+
+ /// 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))?);
+
+ // 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`].
+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
+ // succeeds, fulfilling the invariant of SGTableMap that the object's `sgt` field is
+ // initialized.
+ // SAFETY:
+ // - `obj` is fully initialized, making this function safe to call.
+ from_err_ptr(unsafe { bindings::drm_gem_shmem_get_pages_sgt(obj.as_raw_shmem()) })?;
+
+ Ok(Self { obj: obj.into() })
+ }
+}
+
+// SAFETY: The NonNull in SGTableMap is guaranteed valid by our type invariants, and the GEM object
+// it points to is guaranteed to be thread-safe.
+unsafe impl<T: DriverObject, C: DeviceContext> Send for SGTableMap<T, C> {}
+// SAFETY: The NonNull in SGTableMap is guaranteed valid by our type invariants, and the GEM object
+// it points to is guaranteed to be thread-safe.
+unsafe impl<T: DriverObject, C: DeviceContext> Sync for SGTableMap<T, C> {}
+
#[kunit_tests(rust_drm_gem_shmem)]
mod tests {
use super::*;
@@ -582,4 +710,28 @@ fn vmap_io() -> Result {
Ok(())
}
+
+ // TODO: I would love to actually test the success paths of sg_table(), but that would require
+ // also implementing dummy dma_ops so that trying to create a mapping doesn't explode. So, leave
+ // that for someone else.
+
+ // Ensures that passing the wrong device to sg_table() fails as we expect, and also ensure it
+ // skips initializing `sgt_res` since we could otherwise create `sgt_res` with the wrong device
+ // bound to it.
+ #[test]
+ fn fail_sg_table_on_wrong_dev() -> Result {
+ let (_dev, drm) = create_drm_dev()?;
+ let wrong_dev = faux::Registration::new(c"EvilKunit", None)?;
+
+ let obj = Object::<KunitObject, _>::new(&drm, PAGE_SIZE, ObjectConfig::default(), ())?;
+
+ assert_eq!(obj.sg_table(wrong_dev.as_ref()).err().unwrap(), EINVAL);
+
+ // If sgt_res was not initialized mistakenly with the wrong device, this should still fail.
+ assert_eq!(obj.sg_table(wrong_dev.as_ref()).err().unwrap(), EINVAL);
+
+ // TODO: Someday, we should test that creating an sg_table here still succeeds.
+
+ Ok(())
+ }
}
--
2.54.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Claude review: rust: drm: gem: Introduce shmem::Object::sg_table()
2026-06-04 19:24 ` [PATCH v18 4/4] rust: drm: gem: Introduce shmem::Object::sg_table() Lyude Paul
@ 2026-06-04 20:03 ` Claude Code Review Bot
0 siblings, 0 replies; 18+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 20:03 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Most complex patch — warrants careful scrutiny.**
**Drop ordering in `free_callback` is correct:**
```rust
unsafe { ManuallyDrop::drop(&mut (*this).sgt_res) };
unsafe { bindings::drm_gem_shmem_release(base) };
let _ = unsafe { KBox::from_raw(this) };
```
`sgt_res` must drop first because its `Drop` calls `__drm_gem_shmem_free_sgt_locked` which needs the GEM object alive. Then `drm_gem_shmem_release` cleans up the C-side shmem state. Then `KBox::from_raw` drops the remaining Rust fields. The `ManuallyDrop` pattern is the right choice here since `SetOnce::reset()` was removed in v18.
**Double-checked locking in `sg_table()` is correct:**
```rust
if let Some(sgt_res) = self.sgt_res.as_ref() {
break 'out sgt_res;
}
let _guard = self.sgt_lock.lock();
if let Some(sgt_res) = self.sgt_res.as_ref() {
break 'out sgt_res;
}
self.sgt_res.populate(Devres::new(dev, SGTableMap::new(self))?);
```
This requires that `SetOnce::as_ref()` is safe to call without holding `sgt_lock` (i.e., it provides atomic-like visibility). If `SetOnce` is implemented with proper memory ordering (which kernel `SetOnce` should be), this is correct. If not, there's a data race on the fast path.
**`SGTableMap::new` uses `from_err_ptr` with a `?` inside an `Init` returning closure:**
```rust
fn new(obj: &Object<T, C>) -> impl Init<Self, Error> {
from_err_ptr(unsafe { bindings::drm_gem_shmem_get_pages_sgt(obj.as_raw_shmem()) })?;
Ok(Self { obj: obj.into() })
}
```
This calls `drm_gem_shmem_get_pages_sgt` which acquires/releases `dma_resv` internally (it uses `dma_resv_lock_interruptible`). The return value (an `sg_table*`) is checked for errors via `from_err_ptr` but the actual pointer is then discarded — the implementation relies on the fact that `shmem->sgt` is set as a side effect. This is correct per the C implementation, but the discarded return value deserves a comment explaining this pattern.
**Device identity check:**
```rust
if dev.as_raw() != self.dev().as_ref().as_raw() {
return Err(EINVAL);
}
```
Good — prevents creating a `Devres` bound to the wrong device, which would cause the revocation to fire at the wrong time or not at all.
**`SGTableMap` raw pointer storage:**
```rust
pub struct SGTableMap<T: DriverObject, C: DeviceContext> {
obj: NonNull<Object<T, C>>,
}
```
This stores a non-refcounted pointer to the owning `Object`. The invariant is that `SGTableMap` lives inside the `Object`'s `sgt_res` field, so the object outlives it. The `ManuallyDrop` ensures `sgt_res` is dropped before the object in `free_callback`. This is sound but not self-documenting — a `// INVARIANT:` comment on the `obj` field explaining the lifetime relationship would help future maintainers.
**Test only covers the failure path:**
```rust
fn fail_sg_table_on_wrong_dev() -> Result {
```
The TODO explains that testing the success path would require dummy DMA ops. The failure test is useful — it confirms the device check prevents `sgt_res` from being populated with a wrong device. Good defensive test.
**`Send`/`Sync` for `SGTableMap`:** The safety justification references GEM object thread-safety, which is established by the existing `Send`/`Sync` impls on `Object`. This is correct.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v17 6/6] rust: drm: gem: Introduce shmem::Object::sg_table()
2026-06-03 19:42 [PATCH v17 0/6] Rust bindings for gem shmem Lyude Paul
@ 2026-06-03 19:42 ` Lyude Paul
2026-06-04 1:29 ` Claude review: " Claude Code Review Bot
0 siblings, 1 reply; 18+ messages in thread
From: Lyude Paul @ 2026-06-03 19:42 UTC (permalink / raw)
To: dri-devel, rust-for-linux, nouveau
Cc: Alexandre Courbot, Gary Guo, Christian König, driver-core,
Miguel Ojeda, Maarten Lankhorst, Alice Ryhl, Simona Vetter,
linux-kernel, Sumit Semwal, linux-media, Rafael J . Wysocki,
Thomas Zimmermann, Maxime Ripard, David Airlie, Benno Lossin,
linaro-mm-sig, Danilo Krummrich, Mukesh Kumar Chaurasiya,
Asahi Lina, Daniel Almeida, Lyude Paul, Greg Kroah-Hartman
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 <lyude@redhat.com>
---
V3:
* Rename OwnedSGTable to shmem::SGTable. Since the current version of the
SGTable abstractions now has a `Owned` and `Borrowed` variant, I think
renaming this to shmem::SGTable makes things less confusing.
We do however, keep the name of owned_sg_table() as-is.
V4:
* Clarify safety comments for SGTable to explain why the object is
thread-safe.
* Rename from SGTableRef to SGTable
V10:
* Use Devres in order to ensure that SGTables are revocable, and are
unmapped on driver-unbind.
V11:
* s/create_sg_table()/get_sg_table()
* Get rid of extraneous `ret = ` in shmem::Object::get_sg_table()
V12:
* Actually move sgt_res in this patch and not the next one
V13:
* Use DmaResvGuard suggestion from Alexander
* Use Alexander's (much better) solution for get_sg_table()
* Use SetOnce instead of UnsafeCell
* s/SGTableRef/SGTableMap
* Fix typo in SGTableMap documentation
* Create fallible constructor for SGTableMap
* Don't reuse dma_resv lock for protecting Object contents, just use Mutex
+ SetOnce
* Drop use of drm_gem_shmem_get_pages_sgt_locked(), since we don't need to
hold the dma_resv lock ourselves for anything but this function.
* Check that the device we receive in the bounds for sg_table() and
owned_sg_table() that said Device is in fact, the correct device.
* Remove redundant docs in owned_sg_table(), just point it back to
sg_table().
* Implement Deborah's suggestion to fix double-free in
free_callback()
* Restore original order of Object<T>
* Fix doc typo for SGTableMap
V14:
* Use new InitOnce container over the Mutex/SetOnce horror show we had
before.
* Start using LazyInit container for storing Devres for sgt unmap
* Add some kunit tests for sg_table (not sure why I didn't do this before)
using some of the boilerplate code leftover from the vmap bindings
* Get rid of the owned SGTable variant for now, we'll add it back in a
future patch if people actually need it.
* Use new LazyInit container from me to get rid of the horrid
Mutex<SetOnce<>> mess.
* Add the best we can do for unit tests w/r/t SGTable at the moment
V16:
* Get rid of LazyInit, go back to SetOnce, use trick that Alice recommended
that is a lot cleaner.
* Fix horrid rebasing mistake
V17:
* Rebase
* Fix missing safety comment in free_callback() (we forgot to justify why
&mut is safe in `unsafe { &mut (*this).sgt_res }.reset()`)
rust/kernel/drm/gem/shmem.rs | 166 +++++++++++++++++++++++++++++++++--
1 file changed, 157 insertions(+), 9 deletions(-)
diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
index 1f05a5bc5fe66..dff90771eb34a 100644
--- a/rust/kernel/drm/gem/shmem.rs
+++ b/rust/kernel/drm/gem/shmem.rs
@@ -11,6 +11,11 @@
use crate::{
container_of,
+ device::{
+ self,
+ Bound, //
+ },
+ devres::*,
drm::{
driver,
gem,
@@ -19,14 +24,23 @@
DeviceContext,
Registered, //
},
- error::to_result,
+ error::{
+ from_err_ptr,
+ to_result, //
+ },
io::{
Io,
IoCapable,
IoKnownSize, //
},
prelude::*,
- sync::aref::ARef,
+ scatterlist,
+ sync::{
+ aref::ARef,
+ new_mutex,
+ Mutex,
+ SetOnce, //
+ },
types::Opaque, //
};
use core::{
@@ -87,6 +101,11 @@ pub struct Object<T: DriverObject, C: DeviceContext = Registered> {
obj: Opaque<bindings::drm_gem_shmem_object>,
/// Parent object that owns this object's DMA reservation object.
parent_resv_obj: Option<ARef<Object<T, C>>>,
+ /// Devres object for unmapping any SGTable on driver-unbind.
+ sgt_res: SetOnce<Devres<SGTableMap<T, C>>>,
+ #[pin]
+ /// Lock for protecting initialization of `sgt_res`.
+ sgt_lock: Mutex<()>,
#[pin]
inner: T,
_ctx: PhantomData<C>,
@@ -145,6 +164,8 @@ pub fn new(
try_pin_init!(Self {
obj <- Opaque::init_zeroed(),
parent_resv_obj: config.parent_resv_obj.map(|p| p.into()),
+ sgt_res: SetOnce::new(),
+ sgt_lock <- new_mutex!(()),
inner <- T::new(dev, size, args),
_ctx: PhantomData::<C>,
}),
@@ -189,18 +210,25 @@ extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {
// - DRM always passes a valid gem object here
// - We used drm_gem_shmem_create() in our create_gem_object callback, so we know that
// `obj` is contained within a drm_gem_shmem_object
- let this = unsafe { container_of!(obj, bindings::drm_gem_shmem_object, base) };
-
- // SAFETY:
- // - We're in free_callback - so this function is safe to call.
- // - We won't be using the gem resources on `this` after this call.
- unsafe { bindings::drm_gem_shmem_release(this) };
+ let base = unsafe { container_of!(obj, bindings::drm_gem_shmem_object, base) };
// SAFETY:
// - We verified above that `obj` is valid, which makes `this` valid
// - This function is set in AllocOps, so we know that `this` is contained within a
// `Object<T, C>`
- let this = unsafe { container_of!(Opaque::cast_from(this), Self, obj) }.cast_mut();
+ let this = unsafe { container_of!(Opaque::cast_from(base), Self, obj) }.cast_mut();
+
+ // We need to drop `sgt_res` first, since doing so requires that the GEM object is still
+ // alive.
+ // SAFETY:
+ // - We verified above that `this` is valid.
+ // - We are in free_callback, guaranteeing we have exclusive access to `this`.
+ unsafe { &mut (*this).sgt_res }.reset();
+
+ // SAFETY:
+ // - We're in free_callback - so this function is safe to call.
+ // - We won't be using the gem resources on `this` after this call.
+ unsafe { bindings::drm_gem_shmem_release(base) };
// SAFETY: We're recovering the Kbox<> we created in gem_create_object()
let _ = unsafe { KBox::from_raw(this) };
@@ -279,6 +307,45 @@ pub fn vmap<const SIZE: usize>(&self) -> Result<VMapRef<'_, T, C, SIZE>> {
pub fn owned_vmap<const SIZE: usize>(&self) -> Result<VMapOwned<T, C, SIZE>> {
self.make_vmap()
}
+
+ /// 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))?);
+
+ // 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 +541,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`].
+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
+ // succeeds, fulfilling the invariant of SGTableMap that the object's `sgt` field is
+ // initialized.
+ // SAFETY:
+ // - `obj` is fully initialized, making this function safe to call.
+ from_err_ptr(unsafe { bindings::drm_gem_shmem_get_pages_sgt(obj.as_raw_shmem()) })?;
+
+ Ok(Self { obj: obj.into() })
+ }
+}
+
+// SAFETY: The NonNull in SGTableMap is guaranteed valid by our type invariants, and the GEM object
+// it points to is guaranteed to be thread-safe.
+unsafe impl<T: DriverObject, C: DeviceContext> Send for SGTableMap<T, C> {}
+// SAFETY: The NonNull in SGTableMap is guaranteed valid by our type invariants, and the GEM object
+// it points to is guaranteed to be thread-safe.
+unsafe impl<T: DriverObject, C: DeviceContext> Sync for SGTableMap<T, C> {}
+
#[kunit_tests(rust_drm_gem_shmem)]
mod tests {
use super::*;
@@ -582,4 +706,28 @@ fn vmap_io() -> Result {
Ok(())
}
+
+ // TODO: I would love to actually test the success paths of sg_table(), but that would require
+ // also implementing dummy dma_ops so that trying to create a mapping doesn't explode. So, leave
+ // that for someone else.
+
+ // Ensures that passing the wrong device to sg_table() fails as we expect, and also ensure it
+ // skips initializing `sgt_res` since we could otherwise create `sgt_res` with the wrong device
+ // bound to it.
+ #[test]
+ fn fail_sg_table_on_wrong_dev() -> Result {
+ let (_dev, drm) = create_drm_dev()?;
+ let wrong_dev = faux::Registration::new(c"EvilKunit", None)?;
+
+ let obj = Object::<KunitObject, _>::new(&drm, PAGE_SIZE, ObjectConfig::default(), ())?;
+
+ assert_eq!(obj.sg_table(wrong_dev.as_ref()).err().unwrap(), EINVAL);
+
+ // If sgt_res was not initialized mistakenly with the wrong device, this should still fail.
+ assert_eq!(obj.sg_table(wrong_dev.as_ref()).err().unwrap(), EINVAL);
+
+ // TODO: Someday, we should test that creating an sg_table here still succeeds.
+
+ Ok(())
+ }
}
--
2.54.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Claude review: rust: drm: gem: Introduce shmem::Object::sg_table()
2026-06-03 19:42 ` [PATCH v17 6/6] rust: drm: gem: Introduce shmem::Object::sg_table() Lyude Paul
@ 2026-06-04 1:29 ` Claude Code Review Bot
0 siblings, 0 replies; 18+ messages in thread
From: Claude Code Review Bot @ 2026-06-04 1:29 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Verdict: Has concerns that need resolution.**
This is the most complex patch. The overall design (Devres-managed SGTableMap with double-checked locking) is sound, but there are issues.
**Critical: `__drm_gem_shmem_free_sgt_locked()` does not exist in the current kernel tree.**
```rust
impl<T: DriverObject, C: DeviceContext> Drop for SGTableMap<T, C> {
fn drop(&mut self) {
let obj = unsafe { self.obj.as_ref() };
let _lock = DmaResvGuard::new(obj);
unsafe { bindings::__drm_gem_shmem_free_sgt_locked(obj.as_raw_shmem()) };
}
}
```
I searched the drm-next tree and found no declaration or definition of this function. It must come from a prerequisite patch on drm-rust-next. This function is critical for correctness -- it must NULL out `shmem->sgt` after cleanup, otherwise `drm_gem_shmem_release()` will double-free the sg_table when it runs (it checks `if (shmem->sgt)` and cleans up).
The ordering in `free_callback()` is:
1. `sgt_res.reset()` -- drops SGTableMap, calls `__drm_gem_shmem_free_sgt_locked()`
2. `drm_gem_shmem_release()` -- would double-free if `shmem->sgt` wasn't NULLed
This is a known concern (the V13 changelog mentions "Fix double-free in free_callback()"), but reviewers should verify `__drm_gem_shmem_free_sgt_locked()` NULLs `shmem->sgt`.
**The double-checked locking pattern is correct:**
```rust
pub fn sg_table<'a>(&'a self, dev: &'a device::Device<Bound>) -> Result<&'a scatterlist::SGTable> {
// Fast path
if let Some(sgt_res) = self.sgt_res.as_ref() {
break 'out sgt_res;
}
// Slow path with lock
let _guard = self.sgt_lock.lock();
if let Some(sgt_res) = self.sgt_res.as_ref() {
break 'out sgt_res;
}
self.sgt_res.populate(Devres::new(dev, SGTableMap::new(self))?);
unsafe { self.sgt_res.as_ref().unwrap_unchecked() }
```
The fast path uses `SetOnce::as_ref()` with Acquire ordering, the slow path is guarded by the mutex, and after populate the value is immediately available. This is sound.
**Device identity check is good defensive programming:**
```rust
if dev.as_raw() != self.dev().as_ref().as_raw() {
return Err(EINVAL);
}
```
This prevents creating an SGTable bound to the wrong device, which the test `fail_sg_table_on_wrong_dev` validates.
**SGTableMap self-referential pointer:**
`SGTableMap` stores `NonNull<Object<T, C>>` pointing back to its own containing Object. This is safe because:
- `sgt_res` is explicitly reset in `free_callback()` before `KBox::from_raw(this)` drops the Object
- The Devres revocation on driver-unbind drops the SGTableMap while the Object (refcounted) is still alive
**The `SGTableMap::new()` return type is interesting:**
```rust
fn new(obj: &Object<T, C>) -> impl Init<Self, Error> {
from_err_ptr(unsafe { bindings::drm_gem_shmem_get_pages_sgt(obj.as_raw_shmem()) })?;
Ok(Self { obj: obj.into() })
}
```
This calls `drm_gem_shmem_get_pages_sgt()` which internally acquires/releases the dma_resv lock and initializes `shmem->sgt`. The returned pointer is discarded -- only the side effect (populating the sgt field) matters. This is correct but could use a brief comment explaining why the returned pointer isn't stored.
**Minor: Send/Sync impls:**
```rust
unsafe impl<T: DriverObject, C: DeviceContext> Send for SGTableMap<T, C> {}
unsafe impl<T: DriverObject, C: DeviceContext> Sync for SGTableMap<T, C> {}
```
The safety justification is that the NonNull points to a GEM object which is inherently thread-safe (refcounted, lock-protected). This is reasonable. The `DriverObject` bound already requires `Send + Sync` for the driver data.
**Test coverage:** The test only covers the failure path (wrong device). The TODO acknowledging the need for dummy dma_ops for success testing is honest and reasonable.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v14 6/6] rust: drm: gem: Introduce shmem::Object::sg_table()
2026-05-26 21:28 [PATCH v14 0/6] rust: drm: gem: shmem: Add sg_table() function Lyude Paul
@ 2026-05-26 21:28 ` Lyude Paul
2026-05-27 4:05 ` Claude review: " Claude Code Review Bot
0 siblings, 1 reply; 18+ messages in thread
From: Lyude Paul @ 2026-05-26 21:28 UTC (permalink / raw)
To: dri-devel, rust-for-linux, nouveau
Cc: Alexandre Courbot, Gary Guo, Christian König, driver-core,
Miguel Ojeda, Maarten Lankhorst, Alice Ryhl, Simona Vetter,
linux-kernel, Sumit Semwal, linux-media, Rafael J . Wysocki,
Thomas Zimmermann, Maxime Ripard, David Airlie, Benno Lossin,
linaro-mm-sig, Danilo Krummrich, Mukesh Kumar Chaurasiya,
Asahi Lina, Daniel Almeida, Lyude Paul, Greg Kroah-Hartman
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 <lyude@redhat.com>
---
V3:
* Rename OwnedSGTable to shmem::SGTable. Since the current version of the
SGTable abstractions now has a `Owned` and `Borrowed` variant, I think
renaming this to shmem::SGTable makes things less confusing.
We do however, keep the name of owned_sg_table() as-is.
V4:
* Clarify safety comments for SGTable to explain why the object is
thread-safe.
* Rename from SGTableRef to SGTable
V10:
* Use Devres in order to ensure that SGTables are revocable, and are
unmapped on driver-unbind.
V11:
* s/create_sg_table()/get_sg_table()
* Get rid of extraneous `ret = ` in shmem::Object::get_sg_table()
V12:
* Actually move sgt_res in this patch and not the next one
V13:
* Use DmaResvGuard suggestion from Alexander
* Use Alexander's (much better) solution for get_sg_table()
* Use SetOnce instead of UnsafeCell
* s/SGTableRef/SGTableMap
* Fix typo in SGTableMap documentation
* Create fallible constructor for SGTableMap
* Don't reuse dma_resv lock for protecting Object contents, just use Mutex
+ SetOnce
* Drop use of drm_gem_shmem_get_pages_sgt_locked(), since we don't need to
hold the dma_resv lock ourselves for anything but this function.
* Check that the device we receive in the bounds for sg_table() and
owned_sg_table() that said Device is in fact, the correct device.
* Remove redundant docs in owned_sg_table(), just point it back to
sg_table().
* Implement Deborah's suggestion to fix double-free in
free_callback()
* Restore original order of Object<T>
* Fix doc typo for SGTableMap
V14:
* Use new InitOnce container over the Mutex/SetOnce horror show we had
before.
* Start using LazyInit container for storing Devres for sgt unmap
* Add some kunit tests for sg_table (not sure why I didn't do this before)
using some of the boilerplate code leftover from the vmap bindings
* Get rid of the owned SGTable variant for now, we'll add it back in a
future patch if people actually need it.
* Use new LazyInit container from me to get rid of the horrid
Mutex<SetOnce<>> mess.
* Add the best we can do for unit tests w/r/t SGTable at the moment
rust/kernel/drm/gem/shmem.rs | 214 +++++++++++++++++++++++++++++++++--
1 file changed, 204 insertions(+), 10 deletions(-)
diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
index 1b24cb1129a8b..12307ec6bff07 100644
--- a/rust/kernel/drm/gem/shmem.rs
+++ b/rust/kernel/drm/gem/shmem.rs
@@ -11,15 +11,29 @@
use crate::{
container_of,
+ device::{
+ self,
+ Bound, //
+ },
+ devres::*,
drm::{
driver,
gem,
private::Sealed,
Device, //
},
- error::to_result,
+ error::{
+ from_err_ptr,
+ to_result, //
+ },
prelude::*,
- sync::aref::ARef,
+ scatterlist,
+ sync::{
+ aref::ARef,
+ new_lazy_init,
+ LazyInit,
+ LazyInitError, //
+ },
types::Opaque, //
};
use core::{
@@ -75,6 +89,9 @@ pub struct Object<T: DriverObject> {
obj: Opaque<bindings::drm_gem_shmem_object>,
/// Parent object that owns this object's DMA reservation object.
parent_resv_obj: Option<ARef<Object<T>>>,
+ /// Devres object for unmapping any SGTable on driver-unbind.
+ #[pin]
+ sgt_res: LazyInit<Devres<SGTableMap<T>>>,
#[pin]
inner: T,
}
@@ -127,6 +144,7 @@ pub fn new(
try_pin_init!(Self {
obj <- Opaque::init_zeroed(),
parent_resv_obj: config.parent_resv_obj.map(|p| p.into()),
+ sgt_res <- new_lazy_init!(),
inner <- T::new(dev, size, args),
}),
GFP_KERNEL,
@@ -170,22 +188,49 @@ extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {
// - DRM always passes a valid gem object here
// - We used drm_gem_shmem_create() in our create_gem_object callback, so we know that
// `obj` is contained within a drm_gem_shmem_object
- let this = unsafe { container_of!(obj, bindings::drm_gem_shmem_object, base) };
-
- // SAFETY:
- // - We're in free_callback - so this function is safe to call.
- // - We won't be using the gem resources on `this` after this call.
- unsafe { bindings::drm_gem_shmem_release(this) };
+ let base = unsafe { container_of!(obj, bindings::drm_gem_shmem_object, base) };
// SAFETY:
// - We verified above that `obj` is valid, which makes `this` valid
// - This function is set in AllocOps, so we know that `this` is contained within a
// `Object<T>`
- let this = unsafe { container_of!(Opaque::cast_from(this), Self, obj) }.cast_mut();
+ let this = unsafe { container_of!(Opaque::cast_from(base), Self, obj) }.cast_mut();
+
+ // We need to reset `sgt_res` first, since doing so requires that the GEM object is still
+ // alive.
+ // SAFETY:
+ // - We verified above that `this` is valid.
+ // - `&mut (*this)` does not implement Unpin.
+ unsafe { Pin::new_unchecked(&mut (*this).sgt_res) }.reset();
+
+ // SAFETY:
+ // - We're in free_callback - so this function is safe to call.
+ // - We won't be using the gem resources on `this` after this call.
+ unsafe { bindings::drm_gem_shmem_release(base) };
// SAFETY: We're recovering the Kbox<> we created in gem_create_object()
let _ = unsafe { KBox::from_raw(this) };
}
+
+ /// 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);
+ }
+
+ match self.sgt_res.init(Devres::new(dev, SGTableMap::new(self))) {
+ Ok(ret) | Err(LazyInitError::AlreadyInit(ret)) => Ok(ret.access(dev)?),
+ Err(LazyInitError::DuringInit(e)) => Err(e),
+ }
+ }
}
impl<T: DriverObject> Deref for Object<T> {
@@ -246,7 +291,6 @@ impl<T: DriverObject> driver::AllocImpl for Object<T> {
impl<'a, T: DriverObject> DmaResvGuard<'a, T> {
#[inline(always)]
- #[expect(unused)]
fn new(obj: &'a Object<T>) -> Self {
// SAFETY: This lock is initialized throughout the lifetime of `object`.
unsafe { bindings::dma_resv_lock(obj.raw_dma_resv(), ptr::null_mut()) };
@@ -262,3 +306,153 @@ fn drop(&mut self) {
unsafe { bindings::dma_resv_unlock(self.0.raw_dma_resv()) };
}
}
+
+/// 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`].
+pub struct SGTableMap<T: DriverObject> {
+ obj: NonNull<Object<T>>,
+}
+
+impl<T: DriverObject> Deref for SGTableMap<T> {
+ 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> Drop for SGTableMap<T> {
+ 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> SGTableMap<T> {
+ fn new(obj: &Object<T>) -> impl Init<Self, Error> {
+ // INVARIANT:
+ // - We call drm_gem_shmem_get_pages_sgt_locked below and check whether or not it
+ // succeeds, fulfilling the invariant of SGTableMap that the object's `sgt` field is
+ // initialized.
+ // SAFETY:
+ // - `obj` is fully initialized, making this function safe to call.
+ from_err_ptr(unsafe { bindings::drm_gem_shmem_get_pages_sgt(obj.as_raw_shmem()) })?;
+
+ Ok(Self { obj: obj.into() })
+ }
+}
+
+// SAFETY: The NonNull in SGTableMap is guaranteed valid by our type invariants, and the GEM object
+// it points to is guaranteed to be thread-safe.
+unsafe impl<T: DriverObject> Send for SGTableMap<T> {}
+// SAFETY: The NonNull in SGTableMap is guaranteed valid by our type invariants, and the GEM object
+// it points to is guaranteed to be thread-safe.
+unsafe impl<T: DriverObject> Sync for SGTableMap<T> {}
+
+#[kunit_tests(rust_gem_shmem)]
+mod tests {
+ use super::*;
+ use crate::{
+ drm,
+ faux,
+ page::PAGE_SIZE, //
+ };
+
+ // The bare minimum needed to create a fake drm driver for kunit
+
+ #[pin_data]
+ struct KunitData {}
+ struct KunitDriver;
+ struct KunitFile;
+ #[pin_data]
+ struct KunitObject {}
+
+ const INFO: drm::DriverInfo = drm::DriverInfo {
+ major: 0,
+ minor: 0,
+ patchlevel: 0,
+ name: c"kunit",
+ desc: c"Kunit",
+ };
+
+ impl drm::file::DriverFile for KunitFile {
+ type Driver = KunitDriver;
+
+ fn open(_dev: &drm::Device<KunitDriver>) -> Result<Pin<KBox<Self>>> {
+ Ok(KBox::new(Self, GFP_KERNEL)?.into())
+ }
+ }
+
+ impl gem::DriverObject for KunitObject {
+ type Driver = KunitDriver;
+ type Args = ();
+
+ fn new(
+ _dev: &drm::Device<KunitDriver>,
+ _size: usize,
+ _args: Self::Args,
+ ) -> impl PinInit<Self, Error> {
+ try_pin_init!(KunitObject {})
+ }
+ }
+
+ #[vtable]
+ impl drm::Driver for KunitDriver {
+ type Data = KunitData;
+ type File = KunitFile;
+ type Object = Object<KunitObject>;
+
+ const INFO: drm::DriverInfo = INFO;
+ const IOCTLS: &'static [drm::ioctl::DrmIoctlDescriptor] = &[];
+ }
+
+ fn create_drm_dev(
+ name: &'static CStr,
+ ) -> Result<(faux::Registration, ARef<drm::Device<KunitDriver>>)> {
+ // Create a faux DRM device so we can test gem object creation.
+ let data = try_pin_init!(KunitData {});
+ let dev = faux::Registration::new(name, None)?;
+ let drm = drm::Device::<KunitDriver>::new(dev.as_ref(), data)?;
+
+ Ok((dev, drm))
+ }
+
+ // TODO: I would love to actually test the success paths of sg_table(), but that would require
+ // also implementing dummy dma_ops so that trying to create a mapping doesn't explode. So, leave
+ // that for someone else.
+
+ // Ensures that passing the wrong device to sg_table() fails as we expect, and also ensure it
+ // skips initializing `sgt_res` since we could otherwise create `sgt_res` with the wrong device
+ // bound to it.
+ #[test]
+ fn fail_sg_table_on_wrong_dev() -> Result {
+ let (dev, drm) = create_drm_dev(Some(c"Kunit"))?;
+ let (wrong_dev, wrong_drm) = create_drm_dev(Some(c"EvilKunit"))?;
+
+ let obj = Object::<KunitObject>::new(&drm, PAGE_SIZE, ObjectConfig::default(), ())?;
+
+ assert_eq!(obj.sg_table(wrong_dev.as_ref()).err().unwrap(), EINVAL);
+
+ // If sgt_res was not initialized mistakenly with the wrong device, this should still fail.
+ assert_eq!(obj.sg_table(wrong_dev.as_ref()).err().unwrap(), EINVAL);
+
+ // TODO: Someday, we should test that creating an sg_table here still succeeds.
+
+ Ok(())
+ }
+}
--
2.54.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Claude review: rust: drm: gem: Introduce shmem::Object::sg_table()
2026-05-26 21:28 ` [PATCH v14 6/6] rust: drm: gem: Introduce shmem::Object::sg_table() Lyude Paul
@ 2026-05-27 4:05 ` Claude Code Review Bot
0 siblings, 0 replies; 18+ messages in thread
From: Claude Code Review Bot @ 2026-05-27 4:05 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Status: Issues found, core design is sound**
The overall architecture is well-thought-out:
- `LazyInit<Devres<SGTableMap<T>>>` stores the devres-managed mapping
- `Devres` ensures revocation on driver-unbind
- `LazyInit` ensures one-time initialization
- The self-referential `NonNull<Object<T>>` in `SGTableMap` is safe because `Object` is pinned
**The `free_callback` reordering is correct and important:**
```rust
+ unsafe { Pin::new_unchecked(&mut (*this).sgt_res) }.reset();
+
+ unsafe { bindings::drm_gem_shmem_release(base) };
```
Resetting `sgt_res` before releasing the GEM object ensures the SGTable is freed while the GEM object is still valid. Then `drm_gem_shmem_release` sees `shmem->sgt == NULL` (set by `__drm_gem_shmem_free_sgt_locked`) and skips the now-unnecessary cleanup. No double-free.
**The `sg_table()` method is clean:**
```rust
+ 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);
+ }
+ match self.sgt_res.init(Devres::new(dev, SGTableMap::new(self))) {
+ Ok(ret) | Err(LazyInitError::AlreadyInit(ret)) => Ok(ret.access(dev)?),
+ Err(LazyInitError::DuringInit(e)) => Err(e),
+ }
+ }
```
The device identity check before `init` prevents creating a `Devres` bound to the wrong device. The `LazyInit::init` return handling correctly distinguishes first-init from already-init.
**Issue 1: Test type mismatch.** `create_drm_dev` takes `name: &'static CStr` but is called with `Some(c"Kunit")`:
```rust
+ fn create_drm_dev(
+ name: &'static CStr,
+ ) -> ...
+ let (dev, drm) = create_drm_dev(Some(c"Kunit"))?;
+ let (wrong_dev, wrong_drm) = create_drm_dev(Some(c"EvilKunit"))?;
```
`Some(c"Kunit")` is `Option<&'static CStr>`, not `&'static CStr`. This won't compile. Either the function signature should be `name: Option<&'static CStr>` (if a dependency changed `faux::Registration::new` to take optional name), or the calls should pass `c"Kunit"` directly without `Some()`.
**Issue 2: Unused variable.** `wrong_drm` is bound but never used:
```rust
+ let (wrong_dev, wrong_drm) = create_drm_dev(Some(c"EvilKunit"))?;
```
This will produce a compiler warning. Should be `_wrong_drm` or `_`.
**Minor: SGTableMap::new discards the sgt pointer.** This is intentional (using `drm_gem_shmem_get_pages_sgt` for its side effect of populating `shmem->sgt`), but a brief comment would help readers understand why the result of `from_err_ptr` is discarded:
```rust
+ fn new(obj: &Object<T>) -> impl Init<Self, Error> {
+ from_err_ptr(unsafe { bindings::drm_gem_shmem_get_pages_sgt(obj.as_raw_shmem()) })?;
+ Ok(Self { obj: obj.into() })
+ }
```
The `Send`/`Sync` impls for `SGTableMap` are justified by the GEM object's thread-safety guarantees.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 18+ messages in thread