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

[Alistair Popple]

On 2026-05-07 at 08:06 +1000, Danilo Krummrich <dakr@xxxxxxxxxx> 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.

I don't (yet) claim to be a Rust-for-Linux expert but reading through this made
sense to me and it's been useful for some Nova development so feel free to add:

Reviewed-by: Alistair Popple <apopple@xxxxxxxxxx>

for whatever that is worth :-)

> Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx>
> ---
>  rust/kernel/drm/device.rs | 80 ++++++++++++++++++++++++++++++++++++++-
>  rust/kernel/drm/driver.rs | 10 ++++-
>  2 files changed, 87 insertions(+), 3 deletions(-)
> 
> diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
> index 492c2f2c7ca4..bb685165032d 100644
> --- a/rust/kernel/drm/device.rs
> +++ b/rust/kernel/drm/device.rs
> @@ -6,7 +6,11 @@
>  
>  use crate::{
>      alloc::allocator::Kmalloc,
> -    bindings, device,
> +    bindings,
> +    device::{
> +        self,
> +        AsBusDevice as _, //
> +    },
>      drm::{
>          self,
>          driver::AllocImpl,
> @@ -334,6 +338,80 @@ 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)).
> +    /// Between `drm_dev_enter()` and `drm_dev_exit()` 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>;
> +
> +    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> {
> +    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 9d06f8c5b2da..751a68bb27e1 100644
> --- a/rust/kernel/drm/driver.rs
> +++ b/rust/kernel/drm/driver.rs
> @@ -187,8 +187,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()) };
>      }
>  }
> -- 
> 2.54.0
>