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

[Alistair Popple <apopple@nvidia.com>]

On 2026-05-26 at 16:17 +1000, Alistair Popple <apopple@nvidia.com> wrote...
> On 2026-05-07 at 08:06 +1000, Danilo Krummrich <dakr@kernel.org> 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@nvidia.com>
> 
> for whatever that is worth :-)

Which is definitely not much :) I just realised this has been replaced by a more
recent version of this series with 24 patches, so will use that instead.

 - Alistair

> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > ---
> >  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
> >