Re: [PATCH v2 7/7] rust: drm: make Driver::File lifetime-parameterized

["Gary Guo" <gary@garyguo.net>]

On Sat Nov 7, 2026 at 2:03 AM GMT, Danilo Krummrich wrote:
> Make the DRM file private data lifetime-parameterized, allowing drivers
> to borrow from RegistrationData in their per-file data.
>
> Introduce DriverFile<'a> as a lifetime-parameterized trait that receives
> both the device and registration data in open(). Parametrize File<D> on
> the driver type rather than the file data type, deriving the concrete
> file type through Driver::File<'a>.
>
> Add inner_with() to access the driver file data from a File<D> reference.
>
> Ensure file data is always dropped before registration data:
>
>   - In Registration::drop(), detach driver_priv for all open files under
>     filelist_mutex, then drop the detached data after releasing the mutex
>     and wait for file-data destruction in concurrent postclose_callback()
>     calls via an open_count / WaitQueue pair on drm::Device.
>
>   - In postclose_callback(), skip the drop if driver_priv has already
>     been NULLed by the filelist iteration. The NULL write is visible
>     through the filelist_mutex acquire/release chain in
>     drm_close_helper().
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  drivers/gpu/drm/nova/driver.rs |   2 +-
>  drivers/gpu/drm/nova/file.rs   |  13 ++-
>  drivers/gpu/drm/nova/gem.rs    |   8 +-
>  drivers/gpu/drm/tyr/driver.rs  |   2 +-
>  drivers/gpu/drm/tyr/file.rs    |  11 +-
>  rust/kernel/drm/device.rs      |  36 +++++--
>  rust/kernel/drm/driver.rs      | 113 +++++++++++++++++++--
>  rust/kernel/drm/file.rs        | 180 ++++++++++++++++++++++++---------
>  rust/kernel/drm/gem/mod.rs     |  44 ++++++--
>  rust/kernel/drm/gem/shmem.rs   |   9 +-
>  rust/kernel/drm/ioctl.rs       |   6 +-
>  11 files changed, 330 insertions(+), 94 deletions(-)
>
> diff --git a/drivers/gpu/drm/nova/gem.rs b/drivers/gpu/drm/nova/gem.rs
> index 2b6fe9dc0bfa..2a21ff8ba579 100644
> --- a/drivers/gpu/drm/nova/gem.rs
> +++ b/drivers/gpu/drm/nova/gem.rs
> @@ -11,9 +11,9 @@
>      sync::aref::ARef,
>  };
>  
> -use crate::{
> -    driver::{NovaDevice, NovaDriver},
> -    file::File,
> +use crate::driver::{
> +    NovaDevice,
> +    NovaDriver, //
>  };
>  
>  /// GEM Object inner driver data
> @@ -43,7 +43,7 @@ pub(crate) fn new(dev: &NovaDevice, size: usize) -> Result<ARef<gem::Object<Self
>      /// Look up a GEM object handle for a `File` and return an `ObjectRef` for it.
>      #[inline]
>      pub(crate) fn lookup_handle(
> -        file: &drm::File<File>,
> +        file: &drm::File<NovaDriver>,
>          handle: u32,
>      ) -> Result<ARef<gem::Object<Self>>> {
>          gem::Object::lookup_handle(file, handle)
> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
> index c2b99d67369f..46caf7b336d9 100644
> --- a/drivers/gpu/drm/tyr/driver.rs
> +++ b/drivers/gpu/drm/tyr/driver.rs
> @@ -209,7 +209,7 @@ fn drop(self: Pin<&mut Self>) {}
>  impl drm::Driver for TyrDrmDriver {
>      type Data = ();
>      type RegistrationData<'drm> = TyrDrmRegistrationData<'drm>;
> -    type File = TyrDrmFileData;
> +    type File<'a> = TyrDrmFileData;
>      type Object = Bo;
>      type ParentDevice<Ctx: DeviceContext> = platform::Device<Ctx>;
>  
> diff --git a/drivers/gpu/drm/tyr/file.rs b/drivers/gpu/drm/tyr/file.rs
> index 6850708cfcdd..933a365cb016 100644
> --- a/drivers/gpu/drm/tyr/file.rs
> +++ b/drivers/gpu/drm/tyr/file.rs
> @@ -19,13 +19,16 @@
>  #[pin_data]
>  pub(crate) struct TyrDrmFileData {}
>  
> -/// Convenience type alias for our DRM `File` type
> -pub(crate) type TyrDrmFile = drm::file::File<TyrDrmFileData>;
> +/// Convenience type alias for our DRM `File` type.
> +pub(crate) type TyrDrmFile = drm::file::File<TyrDrmDriver>;
>  
> -impl drm::file::DriverFile for TyrDrmFileData {
> +impl drm::file::DriverFile<'_> for TyrDrmFileData {
>      type Driver = TyrDrmDriver;
>  
> -    fn open(_dev: &drm::Device<Self::Driver>) -> impl PinInit<Self, Error> {
> +    fn open(
> +        _device: &TyrDrmDevice<Registered>,
> +        _reg_data: &TyrDrmRegistrationData<'_>,
> +    ) -> impl PinInit<Self, Error> {
>          Ok(Self {})
>      }
>  }
> diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
> index b8444499b39c..bd66e55b6e17 100644
> --- a/rust/kernel/drm/device.rs
> +++ b/rust/kernel/drm/device.rs
> @@ -15,9 +15,13 @@
>      },
>      error::from_err_ptr,
>      prelude::*,
> -    sync::aref::{
> -        ARef,
> -        AlwaysRefCounted, //
> +    sync::{
> +        aref::{
> +            ARef,
> +            AlwaysRefCounted, //
> +        },
> +        atomic::Atomic,
> +        WaitQueue,
>      },
>      types::{
>          NotThreadSafe,
> @@ -190,7 +194,10 @@ fn deref(&self) -> &Self::Target {
>      }
>  }
>  
> -impl<T: drm::Driver> UnregisteredDevice<T> {
> +impl<T: drm::Driver> UnregisteredDevice<T>
> +where
> +    for<'a> T::File<'a>: drm::file::DriverFile<'a, Driver = T>,

Hmm, shouldn't this bound be on drm::Driver?

Does

    type File<'a>: DriverFile<'a, Driver = Self> + 'a;

not work?

> +{
>      const fn compute_features() -> u32 {
>          let mut features = drm::driver::FEAT_GEM;
>  
> @@ -203,8 +210,8 @@ const fn compute_features() -> u32 {
>  
>      const VTABLE: bindings::drm_driver = drm_legacy_fields! {
>          load: None,
> -        open: Some(drm::File::<T::File>::open_callback),
> -        postclose: Some(drm::File::<T::File>::postclose_callback),
> +        open: Some(drm::File::<T>::open_callback),
> +        postclose: Some(drm::File::<T>::postclose_callback),
>          unload: None,
>          release: Some(Device::<T>::release),
>          master_set: None,
> @@ -333,6 +340,21 @@ pub fn new(
>          // SAFETY: `raw_drm` is valid; no concurrent access before registration.
>          unsafe { (*raw_drm.as_ptr()).registration_data = UnsafeCell::new(NonNull::dangling()) };
>  
> +        // SAFETY: `raw_drm` is valid; no concurrent access before registration.
> +        unsafe { (*raw_drm.as_ptr()).open_count = Atomic::new(0) };
> +
> +        // SAFETY:
> +        // - `raw_drm` is valid and `open_count_wq` is uninitialized; no concurrent access
> +        //   before registration.
> +        // - The field is pinned because the Device is pinned (refcounted, allocated by
> +        //   `__drm_dev_alloc()`, never moved).
> +        unsafe {
> +            pin_init::raw_init(
> +                &raw mut (*raw_drm.as_ptr()).open_count_wq,
> +                crate::new_waitqueue!("drm_open_count"),
> +            )
> +        };
> +
>          // SAFETY: The reference count is one, and now we take ownership of that reference as a
>          // `drm::Device`.
>          // INVARIANT: We just created the device above, but have yet to call `drm_dev_register`.
> @@ -357,6 +379,8 @@ pub struct Device<T: drm::Driver, C: DeviceContext = Normal> {
>      dev: Opaque<bindings::drm_device>,
>      data: T::Data,
>      pub(super) registration_data: UnsafeCell<NonNull<T::RegistrationData<'static>>>,
> +    pub(super) open_count: Atomic<i32>,

32-bit open count is enough if you want to skip the overflow checks. Consider
change to `Atomic<i64>` or `Atomic<u64>` instead.

Or is DRM somehow enforcing that no more than 2**31 files can be open against a
device?

> +    pub(super) open_count_wq: WaitQueue,
>      _ctx: PhantomData<C>,
>  }
>  
> diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs
> index f6db91e81e79..92055c4f9e00 100644
> --- a/rust/kernel/drm/driver.rs
> +++ b/rust/kernel/drm/driver.rs
> @@ -7,10 +7,22 @@
>  use crate::{
>      bindings,
>      device,
> -    drm,
> +    drm::{
> +        self,
> +        file::FileData, //
> +    },
>      error::to_result,
> +    interop::list::clist_create,
>      prelude::*,
> -    sync::aref::ARef, //
> +    sync::{
> +        aref::ARef,
> +        atomic::{
> +            Acquire,
> +            Release, //
> +        },
> +        Mutex, //
> +    },
> +    types::Opaque, //
>  };
>  use core::{
>      marker::PhantomPinned,
> @@ -120,8 +132,14 @@ pub trait Driver {
>      /// The type used to manage memory for this driver.
>      type Object: AllocImpl;
>  
> -    /// The type used to represent a DRM File (client)
> -    type File: drm::file::DriverFile;
> +    /// The type used to represent a DRM File (client).
> +    ///
> +    /// File data may borrow from [`RegistrationData`](Driver::RegistrationData). File data is
> +    /// guaranteed to be dropped before registration data, either when the file is closed or
> +    /// when the device is unregistered, whichever comes first.
> +    ///
> +    /// Drivers implement [`DriverFile`](drm::file::DriverFile) for their file data type.
> +    type File<'a>: 'a;
>  
>      /// The bus device type of the parent device that the DRM device is associated with.
>      type ParentDevice<Ctx: device::DeviceContext>: device::AsBusDevice<Ctx>;
> @@ -197,6 +215,13 @@ pub unsafe fn new<E>(
>                  // SAFETY: `drm` is a valid, initialized but not yet registered DRM device.
>                  let ret = unsafe { bindings::drm_dev_register(drm.as_raw(), 0) };
>                  to_result(ret).inspect_err(|_| {
> +                    // SAFETY:
> +                    // - Failed registration unplugs the device and drains registration guards.
> +                    // - `data` remains initialized until this closure returns.
> +                    // - Failed initialization does not run `Registration::drop()`, so this is
> +                    //   the only file-data cleanup for this device.
> +                    unsafe { Self::drop_file_data(drm) };
> +
>                      // SAFETY: `drm_dev_register()` synchronizes SRCU on failure, so no
>                      // concurrent access to `registration_data` is possible at this point.
>                      unsafe { *drm.registration_data.get() = NonNull::dangling() };
> @@ -209,6 +234,75 @@ pub unsafe fn new<E>(
>      pub fn device(&self) -> &drm::Device<T> {
>          &self.drm
>      }
> +
> +    /// Detaches and drops file data, then waits for concurrent postclose destruction.
> +    ///
> +    /// # Safety
> +    ///
> +    /// - `drm` must be unplugged with all `RegistrationGuard` sections drained.
> +    /// - Its registration data must remain alive until this function returns.
> +    /// - This must be the only call for this device.
> +    unsafe fn drop_file_data(drm: &drm::Device<T>) {
> +        let raw = drm.as_raw();
> +        let mut files = None;
> +
> +        // SAFETY: `filelist_mutex` is initialized by `drm_dev_init()` and remains valid for
> +        // the lifetime of the `struct drm_device`.
> +        let filelist_mutex = unsafe { Mutex::from_raw(&raw mut (*raw).filelist_mutex) };
> +        {
> +            let _guard = filelist_mutex.lock();
> +
> +            // SAFETY:
> +            // - `filelist` is initialized and protected by the held mutex.
> +            // - Its entries are `drm_file`s linked through `lhead`; `Opaque` has the same layout.
> +            let filelist = unsafe {
> +                clist_create!(
> +                    &raw mut (*raw).filelist,
> +                    Opaque<bindings::drm_file>,
> +                    bindings::drm_file,
> +                    lhead
> +                )
> +            };
> +
> +            // Detach the allocations so `postclose()` sees NULL and leaves their destruction to us.
> +            // After `postclose()` returns, the DRM core can free the C file, so the detached list
> +            // must not depend on it. All `RegistrationGuard` sections have already been drained.
> +            for file in filelist.iter() {
> +                // SAFETY: `file` is a valid `drm_file` on this device's filelist.
> +                let priv_ptr = unsafe { (*file.get()).driver_priv };
> +
> +                // SAFETY: Setting `driver_priv` to NULL is visible to `postclose_callback()`
> +                // through the `filelist_mutex` acquire/release chain in `drm_close_helper()`.
> +                unsafe { (*file.get()).driver_priv = core::ptr::null_mut() };
> +
> +                // SAFETY:
> +                // - `priv_ptr` comes from `KBox::into_raw()` in `open_callback()`.
> +                // - The file is still in the list, so `postclose_callback()` has not run. Clearing
> +                //   `driver_priv` transfers ownership here and makes a later postclose skip it.
> +                // - Only the owning pointer is moved; the pinned allocation is dropped in place.
> +                let mut data = unsafe {
> +                    Pin::new_unchecked(KBox::from_raw(
> +                        priv_ptr.cast::<FileData<T::File<'static>>>(),
> +                    ))
> +                };
> +                *data.as_mut().project().next = files.take();
> +                files = Some(data);
> +            }
> +        }
> +
> +        // Run driver destructors without `filelist_mutex`. Take each link before dropping its
> +        // allocation to avoid recursively dropping the entire chain.
> +        while let Some(mut data) = files {
> +            files = data.as_mut().project().next.take();
> +            drop(data);
> +            drm.open_count.fetch_sub(1, Release);
> +        }
> +
> +        // Wait for file-data destruction in concurrent `postclose_callback()` calls. No new opens
> +        // can succeed after unplug, so `open_count` is monotonically decreasing.
> +        drm.open_count_wq
> +            .wait_event(|| drm.open_count.load(Acquire) == 0);
> +    }
>  }
>  
>  // SAFETY: `Registration` doesn't offer any methods or access to fields when shared between
> @@ -230,8 +324,13 @@ fn drop(self: Pin<&mut Self>) {
>          // SAFETY: Safe by the invariant of `ARef<drm::Device<T>>`. The existence of this
>          // `Registration` also guarantees that this `drm::Device` is actually registered.
>          unsafe { bindings::drm_dev_unplug(self.drm.as_raw()) };
> -        // After drm_dev_unplug(), the SRCU barrier guarantees that all RegistrationGuard critical
> -        // sections have completed, so no one holds a reference to reg_data anymore.
> -        // reg_data is dropped here automatically.
> +
> +        // SAFETY:
> +        // - `drm_dev_unplug()` has unplugged the device and drained registration guards.
> +        // - Registration data remains alive until this destructor returns.
> +        // - Registration succeeded, so this destructor performs the only file-data cleanup.
> +        unsafe { Self::drop_file_data(&self.drm) };
> +
> +        // Registration data is dropped here automatically, after all file data has been dropped.
>      }
>  }
> diff --git a/rust/kernel/drm/file.rs b/rust/kernel/drm/file.rs
> index bd5e4ca2977f..2dd0e61718fb 100644
> --- a/rust/kernel/drm/file.rs
> +++ b/rust/kernel/drm/file.rs
> @@ -45,60 +75,110 @@ pub(super) fn as_raw(&self) -> *mut bindings::drm_file {
>          self.0.get()
>      }
>  
> -    fn driver_priv(&self) -> *mut T {
> -        // SAFETY: By the type invariants of `Self`, `self.as_raw()` is always valid.
> -        unsafe { (*self.as_raw()).driver_priv }.cast()
> +    /// Returns the registered DRM device this file was opened on.
> +    #[inline]
> +    pub fn device(&self) -> &drm::Device<D, drm::Registered> {
> +        // SAFETY:
> +        // - The open file keeps its minor and device alive.
> +        // - By the type invariants, the device uses driver `D`.
> +        // - The type invariants require a `RegistrationGuard` for the device throughout this
> +        //   borrow, satisfying the `Registered` context.
> +        unsafe { drm::Device::from_raw((*(*self.as_raw()).minor).dev) }
>      }
>  
> -    /// Return a pinned reference to the driver file structure.
> -    pub fn inner(&self) -> Pin<&T> {
> -        // SAFETY: By the type invariant the pointer `self.as_raw()` points to a valid and opened
> -        // `struct drm_file`, hence `driver_priv` has been properly initialized by `open_callback`.
> -        unsafe { Pin::new_unchecked(&*(self.driver_priv())) }
> +    /// Access the driver file data through a closure.
> +    ///
> +    /// The closure receives a pinned reference to the file data.
> +    ///
> +    /// The outer reference carries the `&self` lifetime while the inner type carries the HRTB
> +    /// lifetime `'a`, implying `'a` outlives `&self`. This allows the closure to coerce covariant
> +    /// sub-fields (e.g. `&'a T` to the caller's lifetime) and return them directly in `R`.
> +    #[inline]
> +    pub fn inner_with<'this, R, F>(&'this self, f: F) -> R
> +    where
> +        F: for<'a> FnOnce(Pin<&'this D::File<'a>>) -> R,
> +    {
> +        // SAFETY: By the type invariants, `self.as_raw()` points to a valid `drm_file`.
> +        let ptr: *const FileData<_> = unsafe { (*self.as_raw()).driver_priv.cast_const().cast() };
> +
> +        // SAFETY:
> +        // - The type invariants keep this file's pinned `FileData` allocation valid for `'this`.
> +        // - The `data` field is structurally pinned.
> +        // - The HRTB `for<'a>` prevents the caller from choosing a concrete lifetime, so shortening
> +        //   is sound regardless of variance.
> +        let data: Pin<&'this D::File<'this>> = unsafe { Pin::new_unchecked(&(*ptr).data) };
> +        f(data)
>      }
>  
>      /// The open callback of a `struct drm_file`.
> +    ///
> +    /// Called from `drm_open()`, which is itself called from `fops_open()`. The latter holds a
> +    /// `RegistrationGuard`, so the device is guaranteed to be registered for the duration of this
> +    /// callback.
>      pub(crate) extern "C" fn open_callback(
>          raw_dev: *mut bindings::drm_device,
>          raw_file: *mut bindings::drm_file,
> -    ) -> core::ffi::c_int {
> -        // SAFETY: A callback from `struct drm_driver::open` guarantees that
> -        // - `raw_dev` is valid pointer to a `struct drm_device`,
> -        // - the corresponding `struct drm_device` has been registered.
> -        let drm = unsafe { drm::Device::from_raw(raw_dev) };
> -
> -        // SAFETY: `raw_file` is a valid pointer to a `struct drm_file`.
> -        let file = unsafe { File::<T>::from_raw(raw_file) };
> -
> -        let init = T::open(drm);
> -        let inner = match KBox::try_pin_init(init, GFP_KERNEL) {
> -            Err(e) => {
> -                return e.to_errno();
> -            }
> -            Ok(i) => i,
> -        };
> -
> -        // SAFETY: This pointer is treated as pinned, and the Drop guarantee is upheld in
> -        // `postclose_callback()`.
> -        let driver_priv = KBox::into_raw(unsafe { Pin::into_inner_unchecked(inner) });
> -
> -        // SAFETY: By the type invariants of `Self`, `self.as_raw()` is always valid.
> -        unsafe { (*file.as_raw()).driver_priv = driver_priv.cast() };
> -
> -        0
> +    ) -> core::ffi::c_int
> +    where
> +        for<'a> D::File<'a>: DriverFile<'a, Driver = D>,
> +    {
> +        // SAFETY:
> +        // - The DRM core guarantees that `raw_dev` is valid.
> +        // - `fops_open()` holds a `RegistrationGuard`, satisfying the `Registered` context.
> +        let dev: &drm::device::Device<D, drm::Registered> =
> +            unsafe { drm::device::Device::from_raw(raw_dev) };
> +
> +        dev.registration_data_with(|reg_data| {
> +            let init = try_pin_init!(FileData {
> +                next: None,
> +                data <- <D::File<'_> as DriverFile<'_>>::open(dev, reg_data),
> +            });
> +            let inner = match KBox::try_pin_init(init, GFP_KERNEL) {
> +                Err(e) => return e.to_errno(),
> +                Ok(i) => i,
> +            };
> +
> +            // SAFETY: The allocation stays pinned until `postclose_callback()` or
> +            // `Registration::drop_file_data()` drops it in place.
> +            let driver_priv = KBox::into_raw(unsafe { Pin::into_inner_unchecked(inner) });
> +
> +            dev.open_count.fetch_add(1, Relaxed);
> +
> +            // SAFETY: `raw_file` is a valid pointer to a `struct drm_file`.
> +            unsafe { (*raw_file).driver_priv = driver_priv.cast() };
> +
> +            0
> +        })
>      }
>  
>      /// The postclose callback of a `struct drm_file`.
>      pub(crate) extern "C" fn postclose_callback(
> -        _raw_dev: *mut bindings::drm_device,
> +        raw_dev: *mut bindings::drm_device,
>          raw_file: *mut bindings::drm_file,
> -    ) {
> -        // SAFETY: This reference won't escape this function
> -        let file = unsafe { File::<T>::from_raw(raw_file) };
> -
> -        // SAFETY: `file.driver_priv` has been created in `open_callback` through `KBox::into_raw`.
> -        let _ = unsafe { KBox::from_raw(file.driver_priv()) };
> +    ) where
> +        for<'a> D::File<'a>: DriverFile<'a, Driver = D>,
> +    {
> +        // SAFETY: `raw_file` is a valid pointer to a `struct drm_file`.
> +        let driver_priv = unsafe { (*raw_file).driver_priv };
> +

I think it's non-trivial on why this code here is correct, so might worth adding
a sentence or two about the case when this happens (because drop_file_data runs
first) and why the access to driver_priv is sychronized (because the pointer is
protected by filelist_mutex, and postclose_callback runs after the file has been
detached from the list).

Best,
Gary

> +        if driver_priv.is_null() {
> +            return;
> +        }
> +
> +        // SAFETY:
> +        // - `driver_priv` comes from `KBox::into_raw()` in `open_callback()`.
> +        // - The file is not on the filelist, and the NULL check rules out ownership having been
> +        //   transferred to `Registration::drop_file_data()`.
> +        // - The pinned data is dropped in place while registration data is still alive, since
> +        //   registration cleanup waits for `open_count` to reach zero.
> +        drop(unsafe { KBox::from_raw(driver_priv.cast::<FileData<D::File<'static>>>()) });
> +
> +        // SAFETY: `raw_dev` is valid for the lifetime of the `struct drm_file`.
> +        let dev: &drm::device::Device<D> = unsafe { drm::device::Device::from_raw(raw_dev) };
> +        if dev.open_count.fetch_sub(1, Release) == 1 {
> +            dev.open_count_wq.wake_up();
> +        }
>      }
>  }
>