On Thu Apr 23, 2026 at 4:01 PM BST, Alexandre Courbot wrote:

Hi Lyude,

On Wed Apr 22, 2026 at 8:52 AM JST, Lyude Paul wrote:

...

In order to do this, we need to be careful to ensure that any interface we expose for scatterlists ensures that any mappings created from one are destroyed on driver-unbind. To do this, we introduce a Devres resource into shmem::Object that we use in order to ensure that we release any SGTable mappings on driver-unbind. We store this in an UnsafeCell and protect access to it using the dma_resv lock that we already have from the shmem gem object, which is the same lock that currently protects drm_gem_object_shmem->sgt.

We also provide two different methods for acquiring an sg table: self.sg_table(), and self.owned_sg_table(). The first function is for short-term uses of mapped SGTables, the second is for callers that need to hold onto the mapped SGTable for an extended period of time. The second variant uses Devres of course, whereas the first simply relies on rust's borrow checker to prevent driver-unbind when using the mapped SGTable.

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

rust/kernel/drm/gem/shmem.rs | 192 ++++++++++++++++++++++++++++++++++- 1 file changed, 190 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs index 11749c36e8695..a477312c8a09b 100644 --- a/rust/kernel/drm/gem/shmem.rs +++ b/rust/kernel/drm/gem/shmem.rs @@ -11,25 +11,38 @@ use crate::{ container_of,

• device::{

•    self,
  

•    Bound, //
  

• },
• devres::*, drm::{ driver, gem, private::Sealed, Device, // },

• error::to_result,

• error::{

•    from_err_ptr,
  

•    to_result, //
  

• }, prelude::*,
• scatterlist, types::{ ARef,

This fails on master:

error[E0432]: unresolved import `crate::sync::ARef`
  --> ../rust/kernel/drm/gem/shmem.rs:36:5
  |
36 |     sync::ARef,
  |     ^^^^^^^^^^ no `ARef` in `sync`

Importing `sync::aref::ARef` seems to be the correct way now.

...

     Opaque, //
 }, //

}; use core::{

• cell::UnsafeCell, ops::{ Deref, DerefMut, // },

• ptr::NonNull,

• ptr::{

•    self,
  

•    NonNull, //
  

• },

}; use gem::{ BaseObjectPrivate, @@ -61,6 +74,11 @@ pub struct ObjectConfig<'a, T: DriverObject> { #[repr(C)] #[pin_data] pub struct Object<T: DriverObject> {

• /// Devres object for unmapping any SGTable on driver-unbind.
• ///
• /// This is protected by the object's dma_resv lock. It needs to be before `obj` to ensure that
• /// it is destroyed before `obj` on `Drop`.
• sgt_res: UnsafeCell<Option<Devres<SGTableMap<T>>>>,

I didn't like this `UnsafeCell<Option>` since the last time, but only figured how to replace it now:

sgt_res: SetOnce<Devres<SGTableMap<T>>>,

It's actually designed for that! And lets you remove at least one unsafe statement, while simplifying `get_sg_table` quite a bit. With the other suggestions I have below, here is my version of `get_sg_table` for reference:

fn get_sg_table<'a>(
    &'a self,
    dev: &'a device::Device<Bound>,
) -> Result<&'a Devres<SGTableMap<T>>> {
    let _dma_resv = DmaResvGuard::new(self);

    if let Some(devres) = self.sgt_res.as_ref() {
        Ok(devres)
    } else {
        // Only called for the side-effect of populating the GEM SG table.
        // SAFETY: We grabbed the lock required for calling this function above.
        from_err_ptr(unsafe {
            bindings::drm_gem_shmem_get_pages_sgt_locked(self.as_raw_shmem())
        })?;

        // INVARIANT:
        // - We called drm_gem_shmem_get_pages_sgt_locked above and checked that it
        //   succeeded, fulfilling the invariant of `SGTableMap` that the object's `sgt` field
        //   is initialized.
        // - We store this Devres in the object itself and don't move it, ensuring that the
        //   object it points to remains valid for the lifetime of the `SGTableMap`.
        let devres =
            Devres::new(dev, init!(SGTableMap { obj: self.into() })).inspect_err(|_| {
                // We can't make sure that the pages for this object are unmapped on
                // driver-unbind, so we need to release the sgt
                // SAFETY:
                // - We grabbed the lock required for calling this function above
                // - We checked above that get_pages_sgt_locked() was successful
                unsafe { bindings::__drm_gem_shmem_free_sgt_locked(self.as_raw_shmem()) }
            })?;

        self.sgt_res.populate(devres);

        // PANIC: `populate` has just succeeded, guaranteeing that `sgt_res` is populated.
        Ok(self.sgt_res.as_ref().unwrap())
    }
}

And if only we could populate the `SetOnce` with a `impl Init<T, E>`, then we could even remove the DMA reservation acquisition on the fast path, because `SetOnce` comes with its own locking and the DMA lock here is used outside of its intended scope. I'll try to push the necessary work for `SetOnce` and maybe we can do that as a follow-up patch.

I have this sitting in my once_wip branch for while https://github.com/nbdd0121/linux/commits/once_wip/ (the specific commit that adds init support is https://github.com/nbdd0121/linux/commit/4aabdbcf20b11626c253f203745b1d55c37...).

This was implemented for lazy revocable support which Alvin has picked up, see https://lore.kernel.org/rust-for-linux/20260326-b4-tyr-debugfs-v1-1-074badd1...

Best, Gary

On Fri Apr 24, 2026 at 12:01 AM JST, Alexandre Courbot wrote:

Hi Lyude,

On Wed Apr 22, 2026 at 8:52 AM JST, Lyude Paul wrote:

...

In order to do this, we need to be careful to ensure that any interface we expose for scatterlists ensures that any mappings created from one are destroyed on driver-unbind. To do this, we introduce a Devres resource into shmem::Object that we use in order to ensure that we release any SGTable mappings on driver-unbind. We store this in an UnsafeCell and protect access to it using the dma_resv lock that we already have from the shmem gem object, which is the same lock that currently protects drm_gem_object_shmem->sgt.

We also provide two different methods for acquiring an sg table: self.sg_table(), and self.owned_sg_table(). The first function is for short-term uses of mapped SGTables, the second is for callers that need to hold onto the mapped SGTable for an extended period of time. The second variant uses Devres of course, whereas the first simply relies on rust's borrow checker to prevent driver-unbind when using the mapped SGTable.

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

rust/kernel/drm/gem/shmem.rs | 192 ++++++++++++++++++++++++++++++++++- 1 file changed, 190 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs index 11749c36e8695..a477312c8a09b 100644 --- a/rust/kernel/drm/gem/shmem.rs +++ b/rust/kernel/drm/gem/shmem.rs @@ -11,25 +11,38 @@ use crate::{ container_of,

• device::{

•    self,
  

•    Bound, //
  

• },
• devres::*, drm::{ driver, gem, private::Sealed, Device, // },

• error::to_result,

• error::{

•    from_err_ptr,
  

•    to_result, //
  

• }, prelude::*,
• scatterlist, types::{ ARef,

This fails on master:

error[E0432]: unresolved import `crate::sync::ARef`
  --> ../rust/kernel/drm/gem/shmem.rs:36:5
  |
36 |     sync::ARef,
  |     ^^^^^^^^^^ no `ARef` in `sync`

Importing `sync::aref::ARef` seems to be the correct way now.

...

     Opaque, //
 }, //

}; use core::{

• cell::UnsafeCell, ops::{ Deref, DerefMut, // },

• ptr::NonNull,

• ptr::{

•    self,
  

•    NonNull, //
  

• },

}; use gem::{ BaseObjectPrivate, @@ -61,6 +74,11 @@ pub struct ObjectConfig<'a, T: DriverObject> { #[repr(C)] #[pin_data] pub struct Object<T: DriverObject> {

• /// Devres object for unmapping any SGTable on driver-unbind.
• ///
• /// This is protected by the object's dma_resv lock. It needs to be before `obj` to ensure that
• /// it is destroyed before `obj` on `Drop`.
• sgt_res: UnsafeCell<Option<Devres<SGTableMap<T>>>>,

I didn't like this `UnsafeCell<Option>` since the last time, but only figured how to replace it now:

sgt_res: SetOnce<Devres<SGTableMap<T>>>,

It's actually designed for that! And lets you remove at least one unsafe statement, while simplifying `get_sg_table` quite a bit. With the other suggestions I have below, here is my version of `get_sg_table` for reference:

fn get_sg_table<'a>(
    &'a self,
    dev: &'a device::Device<Bound>,
) -> Result<&'a Devres<SGTableMap<T>>> {
    let _dma_resv = DmaResvGuard::new(self);

    if let Some(devres) = self.sgt_res.as_ref() {
        Ok(devres)
    } else {
        // Only called for the side-effect of populating the GEM SG table.
        // SAFETY: We grabbed the lock required for calling this function above.
        from_err_ptr(unsafe {
            bindings::drm_gem_shmem_get_pages_sgt_locked(self.as_raw_shmem())
        })?;

        // INVARIANT:
        // - We called drm_gem_shmem_get_pages_sgt_locked above and checked that it
        //   succeeded, fulfilling the invariant of `SGTableMap` that the object's `sgt` field
        //   is initialized.
        // - We store this Devres in the object itself and don't move it, ensuring that the
        //   object it points to remains valid for the lifetime of the `SGTableMap`.
        let devres =
            Devres::new(dev, init!(SGTableMap { obj: self.into() })).inspect_err(|_| {
                // We can't make sure that the pages for this object are unmapped on
                // driver-unbind, so we need to release the sgt
                // SAFETY:
                // - We grabbed the lock required for calling this function above
                // - We checked above that get_pages_sgt_locked() was successful
                unsafe { bindings::__drm_gem_shmem_free_sgt_locked(self.as_raw_shmem()) }
            })?;

        self.sgt_res.populate(devres);

        // PANIC: `populate` has just succeeded, guaranteeing that `sgt_res` is populated.
        Ok(self.sgt_res.as_ref().unwrap())
    }
}

And if only we could populate the `SetOnce` with a `impl Init<T, E>`, then we could even remove the DMA reservation acquisition on the fast path, because `SetOnce` comes with its own locking and the DMA lock here is used outside of its intended scope. I'll try to push the necessary work for `SetOnce` and maybe we can do that as a follow-up patch.

...

 #[pin]
 obj: Opaque<bindings::drm_gem_shmem_object>,
 /// Parent object that owns this object's DMA reservation object.

@@ -117,6 +135,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: UnsafeCell::new(None),
             inner <- T::new(dev, size, args),
         }),
         GFP_KERNEL,
  

@@ -176,6 +195,100 @@ 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) }; }

• // If necessary, create an SGTable for the gem object and register a Devres for it to ensure
• // that it is unmapped on driver unbind.
• fn get_sg_table<'a>(

•    &'a self,
  

•    dev: &'a device::Device<Bound>,
  

• ) -> Result<&'a Devres<SGTableMap<T>>> {

•    let sgt_res_ptr = self.sgt_res.get();
  

•    // SAFETY: This lock is initialized throughout the lifetime of the gem object
  

•    unsafe { bindings::dma_resv_lock(self.raw_dma_resv(), ptr::null_mut()) };
  

There are 4 sites where we acquire and release the DMA resv lock, each of which require unsafe blocks and carrying the risk that we forget releasing the lock in the end. For this method in particular we need to jump through hoops a bit and store the return value into a temporary variable so we can unlock the DMA reservation.

Let's do ourselves a favor and implement a small, private guard type:

struct DmaResvGuard<'a, T: DriverObject>(&'a Object<T>);

impl<'a, T: DriverObject> DmaResvGuard<'a, T> {
    fn new(object: &'a Object<T>) -> Self {
        // SAFETY: This lock is initialized throughout the lifetime of `object`
        unsafe { bindings::dma_resv_lock(object.raw_dma_resv(), ptr::null_mut()) };

        Self(object)
    }
}

impl<'a, T> Drop for DmaResvGuard<'a, T>
where
    T: DriverObject,
{
    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()) };
    }
}

There here you would just do

let _dma_resv = DmaResvGuard::new(self);

and write the rest of the method without without having to worry about not returning early. It also let's you improve the flow of the code a bit, and requires less unsafe blocks overall.

I am not sure how much of the TODO at the beginning of the file this solves, but it should also make it easier to switch to something that acquires a reference to a Wwmutex.

...

•    // SAFETY: We just grabbed the lock required for reading this data above.
  

•    let sgt_res = unsafe { (*sgt_res_ptr).as_ref() };
  

•    let ret = if let Some(sgt_res) = sgt_res {
  

•        // We already have a Devres object for this sg table, return it
  

•        Ok(sgt_res)
  

•    } else {
  

•        // SAFETY: We grabbed the lock required for calling this function above */
  

•        let sgt = from_err_ptr(unsafe {
  

•            bindings::drm_gem_shmem_get_pages_sgt_locked(self.as_raw_shmem())
  

•        });
  

•        if let Err(e) = sgt {
  

•            Err(e)
  

•        } else {
  

•            // INVARIANT:
  

•            // - We called drm_gem_shmem_get_pages_sgt_locked above and checked that it
  

•            //   succeeded, fulfilling the invariant of SGTableRef that the object's `sgt` field
  

s/SGTableRef/SGTableMap? (several like this through the patch).

...

•            //   is initialized.
  

•            // - We store this Devres in the object itself and don't move it, ensuring that the
  

•            //   object it points to remains valid for the lifetime of the SGTableRef.
  

•            let devres = Devres::new(dev, init!(SGTableMap { obj: self.into() }));
  

•            match devres {
  

•                Ok(devres) => {
  

•                    // SAFETY: We acquired the lock protecting this data above, making it safe
  

•                    // to write into here
  

•                    unsafe { (*sgt_res_ptr) = Some(devres) };
  

•                    // SAFETY: We just write Some() into *sgt_res_ptr above
  

•                    Ok(unsafe { (&*sgt_res_ptr).as_ref().unwrap_unchecked() })
  

•                }
  

•                Err(e) => {
  

•                    // We can't make sure that the pages for this object are unmapped on
  

•                    // driver-unbind, so we need to release the sgt
  

•                    // SAFETY:
  

•                    // - We grabbed the lock required for calling this function above
  

•                    // - We checked above that get_pages_sgt_locked() was successful
  

•                    unsafe { bindings::__drm_gem_shmem_free_sgt_locked(self.as_raw_shmem()) };
  

•                    Err(e)
  

•                }
  

•            }
  

•        }
  

•    };
  

•    // SAFETY: We're releasing the lock that we grabbed above.
  

•    unsafe { bindings::dma_resv_unlock(self.raw_dma_resv()) };
  

•    ret
  

• }
• /// 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.
• #[inline]
• pub fn sg_table<'a>(

•    &'a self,
  

•    dev: &'a device::Device<Bound>,
  

• ) -> Result<&'a scatterlist::SGTable> {

•    let sgt = self.get_sg_table(dev)?;
  

•    Ok(sgt.access(dev)?.deref())
  

• }
• /// Creates (if necessary) and returns an owned reference to a scatter-gather table of DMA pages
• /// for this object.
• ///
• /// This is the same as [`sg_table`](Self::sg_table), except that it instead returns an
• /// [`shmem::SGTable`] which holds a reference to the associated gem object, instead of a
• /// reference to an [`scatterlist::SGTable`].
• ///
• /// This will pin the object in memory.
• ///
• /// [`shmem::SGTable`]: SGTable
• pub fn owned_sg_table(&self, dev: &device::Device<Bound>) -> Result<SGTable<T>> {

•    self.get_sg_table(dev)?;
  

•    // INVARIANT: We just ensured above that `self.sgt_res` is initialized with
  

•    // `Some(Devres<SGTableMap<T>>)`.
  

•    Ok(SGTable(self.into()))
  

• }

} impl<T: DriverObject> Deref for Object<T> { @@ -226,3 +339,78 @@ impl<T: DriverObject> driver::AllocImpl for Object<T> { dumb_map_offset: None, }; }

+/// 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.0.owner.sgt` has an initialized and valid SGTable.

The comment mentions `self.0` and "a valid SGTable", which don't seem to apply to this type.

...

+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() };
  

•    // SAFETY: The dma_resv for GEM objects is initialized throughout its lifetime
  

•    unsafe { bindings::dma_resv_lock(obj.raw_dma_resv(), ptr::null_mut()) };
  

•    // SAFETY: We acquired the lock needed for calling this function above
  

•    unsafe { bindings::__drm_gem_shmem_free_sgt_locked(obj.as_raw_shmem()) };
  

For symmetry, I wanted to suggest moving the call to `drm_gem_shmem_get_pages_sgt_locked` to a (fallible) constructor of `SGTableMap`.

It's not only for cosmetic and code proximity reasons - once you bind the call to `drm_gem_shmem_get_pages_sgt_locked` to the successful creation of the `SGTableMap` object, you can remove the custom error path of `get_sg_table` that called `__drm_gem_shmem_free_sgt_locked` if `Devres::new` failed since the destructor of `SGTableMap` will now take care of this for us. This simplifies the last complex bit of `get_sg_table`.

The problem is that it would also normally require `SGTableMap::new` to acquire the DMA reservation lock in order to call `drm_gem_shmem_get_pages_sgt_locked`, but we already have it in `get_sg_table`.

If we stopped using the DMA reservation lock as a lock for population `sgt_res` and instead switched to a regular Mutex, we could then move the DMA reservation acquisition to the constructor, attain symmetry, and simplify `get_sg_table` to the point where it becomes trivial. That use is also fragile as the `SGTableMap` destructor acquires it, so we must be very careful to never drop it in `get_sg_table`.

I think that would be a good tradeoff for the time until we make `SetOnce` capable of being populated using an `impl Init`.

Another benefit (I guess) of using a dedicated lock is that you could also call `drm_gem_shmem_get_pages_sgt`, removing the need for patch 3.

On Wed Apr 22, 2026 at 8:52 AM JST, Lyude Paul wrote: <snip>

@@ -176,6 +195,100 @@ 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) }; }

• // If necessary, create an SGTable for the gem object and register a Devres for it to ensure
• // that it is unmapped on driver unbind.
• fn get_sg_table<'a>(

•    &'a self,
  

•    dev: &'a device::Device<Bound>,
  

Just noticed that the caller can technically pass a different `dev` from the one the buffer will be mapped onto, potentially linking the `Devres` (and thus the device mapping) to a completely unrelated lifetime.

I.e. `dev` is only used to create the `Devres`, while `drm_gem_shmem_get_pages_sgt_locked` uses the object's device pointer to create the mapping. We need to add the check ensuring that the two point to the same device:

let expected = self.dev().as_ref().as_raw(); if dev.as_raw() != expected { return Err(EINVAL); }

Ideally we could do without the `dev` argument altogether, but that's our only guarantee that the device is bound, so I don't think we can do without this runtime check.

I think it's also worth mentioning in the documentations of `sg_table` and `owned_sg_table` which device is expected.

On Wed Apr 22, 2026 at 8:52 AM JST, Lyude Paul wrote:

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.

Similar to how we handle SGTables, we make sure there's two different types of mappings: owned mappings (kernel::drm::gem::shmem::VMap) and borrowed mappings (kernel::drm::gem::shmem::VMapRef).

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.

I am not sure this is enough to solve the double-free issue - although my GEM knowledge is lacking, so please take this with a grain of salt.

Take an object where we called `sg_table`, so the SGT has been created.

When the object is dropped by DRM, `free_callback` is first called. It calls `drm_gem_shmem_release`, which frees the SGT.

Then, the `Drop` implementation of `Object` is called, and `sgt_res` is dropped... which attempts to free the SGT again.

I haven't tested that and rely only on a quick look at the code and my partial understanding, but the correct fix appears to be to clearing `sgt_res` in `free_callback`.

• Fix typo in Object.raw_vmap()

rust/kernel/drm/gem/shmem.rs | 355 +++++++++++++++++++++++++++++++++++ 1 file changed, 355 insertions(+)

diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs index a477312c8a09b..b96de8d33141d 100644 --- a/rust/kernel/drm/gem/shmem.rs +++ b/rust/kernel/drm/gem/shmem.rs @@ -26,6 +26,11 @@ from_err_ptr, to_result, // },

• io::{

•    Io,
  

•    IoCapable,
  

•    IoKnownSize, //
  

• }, prelude::*, scatterlist, types::{

@@ -35,6 +40,11 @@ }; use core::{ cell::UnsafeCell,

• ffi::c_void,
• mem::{

•    self,
  

•    MaybeUninit, //
  

• }, ops::{ Deref, DerefMut, //

@@ -45,6 +55,7 @@ }, }; use gem::{

• BaseObject, BaseObjectPrivate, DriverObject, IntoGEMObject, //

@@ -289,6 +300,84 @@ pub fn owned_sg_table(&self, dev: &device::Device<Bound>) -> Result<SGTable<T>> // `Some(Devres<SGTableMap<T>>)`. Ok(SGTable(self.into())) }

• /// Attempt to create a vmap from the gem object, and confirm the size of said vmap.
• fn raw_vmap(&self, min_size: usize) -> Result<*mut c_void> {

•    if self.size() < min_size {
  

•        return Err(ENOSPC);
  

•    }
  

•    let mut map: MaybeUninit<bindings::iosys_map> = MaybeUninit::uninit();
  

•    // SAFETY: drm_gem_shmem_vmap can be called with the DMA reservation lock held
  

•    to_result(unsafe {
  

•        // TODO: see top of file
  

•        bindings::dma_resv_lock(self.raw_dma_resv(), ptr::null_mut());
  

•        let ret = bindings::drm_gem_shmem_vmap_locked(self.as_raw_shmem(), map.as_mut_ptr());
  

•        bindings::dma_resv_unlock(self.raw_dma_resv());
  

•        ret
  

•    })?;
  

•    // 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() };
  

With the DMA reservation guard I suggested in the previous patch, this can become

let map = { let mut map: MaybeUninitbindings::iosys_map = MaybeUninit::uninit(); let _dma_resv = 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()) })?;

// SAFETY: The call to drm_gem_shmem_vmap_locked succeeded above, so we are guaranteed that // map is properly initialized. 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 {
  

•        // SAFETY: We checked that this is not an iomem allocation, making it safe to read vaddr
  

•        Ok(unsafe { map.__bindgen_anon_1.vaddr })
  

•    }
  

• }
• /// 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 resv = self.raw_dma_resv();
  

•    // 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 {
  

•        // TODO: see top of file
  

•        bindings::dma_resv_lock(resv, ptr::null_mut());
  

•        bindings::drm_gem_shmem_vunmap_locked(self.as_raw_shmem(), &mut map);
  

•        bindings::dma_resv_unlock(resv);
  

•    }
  

• }
• /// Creates and returns a virtual kernel memory mapping for this object.
• #[inline]
• pub fn vmap<const SIZE: usize>(&self) -> Result<VMapRef<'_, T, SIZE>> {

•    Ok(VMap {
  

•        // INVARIANT: `raw_vmap()` checks that the gem object is at least as large as `SIZE`.
  

•        addr: self.raw_vmap(SIZE)?,
  

•        owner: self,
  

•    })
  

• }
• /// 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>> {

•    Ok(VMap {
  

•        // INVARIANT: `raw_vmap()` checks that the gem object is at least as large as `SIZE`.
  

•        addr: self.raw_vmap(SIZE)?,
  

•        owner: self.into(),
  

•    })
  

• }

You can factor out some of the code into a generic private method:

fn make_vmap<R, const SIZE: usize>(&self, owner: R) -> Result<VMap<T, R, SIZE>> where R: Deref<Target = Object<T>>, { Ok(VMap { // INVARIANT: `raw_vmap()` checks that the gem object is at least as large as `SIZE`. addr: self.raw_vmap(SIZE)?, owner, }) }

/// 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(self) }

/// 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(self.into()) }

It doesn't result in less LoCs, but reduces duplication and removes one `INVARIANT` statement.