Re: [PATCH v2 2/7] rust: drm: Add UnbindGuard for drm_dev_enter/exit critical sections

[Gary Guo]

On Wed Jun 3, 2026 at 2:15 AM BST, Danilo Krummrich wrote:
> DRM ioctls do not guarantee that the parent bus device is still bound.
> However, since DRM device registration is managed through Devres, using
> drm_dev_unplug() on unregistration ensures that between drm_dev_enter()
> and drm_dev_exit() the parent device must be bound.
>
> Add UnbindGuard, a guard object representing a drm_dev_enter/exit SRCU
> critical section that dereferences to &Device<Bound> of the parent bus
> device. The guard is only available on Device<T, Registered>, ensuring
> it cannot be used on unregistered devices.
>
> Also add with_unbind_guard() as a convenience helper that executes a
> closure with the bound device reference.
>
> Switch Registration::drop from drm_dev_unregister() to drm_dev_unplug()
> to provide the SRCU barrier that UnbindGuard's safety argument relies on.


Conceptually the actual thing this is guarding against is not device unbind but
DRM unregistration. Currently we force DRM registration to use
`devres::register` so they're the same, but with patch 3 you're adding
registration data and `Registration` is now something that lives in parent
device's driver data.

This also means that it is *possible* that a `Registration` is dropped before
device unbinding, so I think the `UnbindGuard` is not the best name for this
now, something like `UnregistrationGuard` reflect what it does more.

Best,
Gary

>
> Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx>
> ---
>  rust/kernel/drm/device.rs | 82 ++++++++++++++++++++++++++++++++++++++-
>  rust/kernel/drm/driver.rs | 10 ++++-
>  2 files changed, 89 insertions(+), 3 deletions(-)
>
> diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
> index 858272fc2164..828618ae19af 100644
> --- a/rust/kernel/drm/device.rs
> +++ b/rust/kernel/drm/device.rs
> @@ -7,7 +7,10 @@
>  use crate::{
>      alloc::allocator::Kmalloc,
>      bindings,
> -    device,
> +    device::{
> +        self,
> +        AsBusDevice as _, //
> +    },
>      drm::{
>          self,
>          driver::AllocImpl,
> @@ -355,6 +358,83 @@ pub(crate) unsafe fn assume_ctx<NewCtx: DeviceContext>(&self) -> &Device<T, NewC
>      }
>  }
>  
> +impl<T: drm::Driver> Device<T, Registered> {
> +    /// Guard against the parent bus device being unbound.
> +    ///
> +    /// Returns an [`UnbindGuard`] if the device has not been unplugged, [`None`] otherwise.
> +    ///
> +    /// The returned guard dereferences to the parent bus device in the [`device::Bound`] context
> +    /// (see [`Driver::ParentDevice`](drm::Driver::ParentDevice)).
> +    ///
> +    /// While [`UnbindGuard`] is held the parent device is guaranteed to be bound.
> +    #[must_use]
> +    pub fn unbind_guard(&self) -> Option<UnbindGuard<'_, T>> {
> +        let mut idx: i32 = 0;
> +        // SAFETY: `self.as_raw()` is a valid pointer to a `struct drm_device` by the type
> +        // invariants of `Device<T, Registered>`.
> +        if unsafe { bindings::drm_dev_enter(self.as_raw(), &mut idx) } {
> +            Some(UnbindGuard { dev: self, idx })
> +        } else {
> +            None
> +        }
> +    }
> +
> +    /// Execute a closure while the parent bus device is guaranteed to be bound.
> +    ///
> +    /// Acquires the [`UnbindGuard`] and, if the device has not been unplugged, calls `f` with the
> +    /// parent bus device. Returns `None` if the device has been unplugged.
> +    pub fn with_unbind_guard<R>(
> +        &self,
> +        f: impl FnOnce(&T::ParentDevice<device::Bound>) -> R,
> +    ) -> Option<R> {
> +        let guard = self.unbind_guard()?;
> +        Some(f(&guard))
> +    }
> +}
> +
> +/// A guard preventing the parent bus device from being unbound.
> +///
> +/// The guard dereferences to [`Driver::ParentDevice<Bound>`](drm::Driver::ParentDevice), providing
> +/// access to the parent bus device with the guarantee that it is bound for the entire duration of
> +/// the critical section.
> +///
> +/// Internally this is backed by a `drm_dev_enter()` / `drm_dev_exit()` SRCU critical section.
> +///
> +/// See [`Device::unbind_guard`] for details on the safety argument.
> +///
> +/// # Invariants
> +///
> +/// - `idx` is the SRCU read lock index returned by a successful `drm_dev_enter()` call.
> +/// - The parent bus device of `dev` is bound for the lifetime of this guard.
> +#[must_use]
> +pub struct UnbindGuard<'a, T: drm::Driver> {
> +    dev: &'a Device<T, Registered>,
> +    idx: i32,
> +}
> +
> +impl<T: drm::Driver> Deref for UnbindGuard<'_, T> {
> +    type Target = T::ParentDevice<device::Bound>;
> +
> +    #[inline]
> +    fn deref(&self) -> &Self::Target {
> +        // SAFETY:
> +        // - The parent `struct device` is embedded in a `T::ParentDevice`, as guaranteed by
> +        //   `UnregisteredDevice::new` taking a `&T::ParentDevice<device::Bound>`.
> +        // - By the type invariants of `UnbindGuard`, the parent device is bound for the lifetime
> +        //   of this guard.
> +        unsafe { T::ParentDevice::from_device(self.dev.as_ref().as_bound()) }
> +    }
> +}
> +
> +impl<T: drm::Driver> Drop for UnbindGuard<'_, T> {
> +    #[inline]
> +    fn drop(&mut self) {
> +        // SAFETY: `self.idx` was returned by a successful `drm_dev_enter()` call, as guaranteed
> +        // by the type invariants of `UnbindGuard`.
> +        unsafe { bindings::drm_dev_exit(self.idx) };
> +    }
> +}
> +
>  impl<T: drm::Driver, C: DeviceContext> Deref for Device<T, C> {
>      type Target = T::Data;
>  
> diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs
> index 802e7fc13e30..f68a17d8939d 100644
> --- a/rust/kernel/drm/driver.rs
> +++ b/rust/kernel/drm/driver.rs
> @@ -199,8 +199,14 @@ unsafe impl<T: Driver> Send for Registration<T> {}
>  
>  impl<T: Driver> Drop for Registration<T> {
>      fn drop(&mut self) {
> +        // Use `drm_dev_unplug` rather than `drm_dev_unregister` to ensure that existing
> +        // `drm_dev_enter()` critical sections complete before unregistration proceeds. This
> +        // is required for the safety of `UnbindGuard`, which relies on the SRCU barrier in
> +        // `drm_dev_unplug()` to guarantee that the parent device is still bound within the
> +        // critical section.
> +        //
>          // SAFETY: Safe by the invariant of `ARef<drm::Device<T>>`. The existence of this
> -        // `Registration` also guarantees the this `drm::Device` is actually registered.
> -        unsafe { bindings::drm_dev_unregister(self.0.as_raw()) };
> +        // `Registration` also guarantees that this `drm::Device` is actually registered.
> +        unsafe { bindings::drm_dev_unplug(self.0.as_raw()) };
>      }
>  }