Re: [PATCH v2 1/3] rust/drm: Introduce DeviceContext

["Danilo Krummrich" <dakr@kernel.org>]

On Fri Jan 16, 2026 at 9:41 PM CET, Lyude Paul wrote:
> diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
> index 3ce8f62a00569..53ca71daf2f86 100644
> --- a/rust/kernel/drm/device.rs
> +++ b/rust/kernel/drm/device.rs
> @@ -7,14 +7,20 @@
>  use crate::{
>      alloc::allocator::Kmalloc,
>      bindings, device, drm,
> -    drm::driver::AllocImpl,
> +    drm::{driver::AllocImpl, private::Sealed},

Please use kernel vertical style [1] when making changes.

(If you want you can also add an additional patch converting the whole DRM
module first for consistency.)

[1] https://docs.kernel.org/rust/coding-guidelines.html#imports

>      error::from_err_ptr,
>      error::Result,
>      prelude::*,
>      sync::aref::{ARef, AlwaysRefCounted},
> -    types::Opaque,
> +    types::{NotThreadSafe, Opaque},
> +};
> +use core::{
> +    alloc::Layout,
> +    marker::PhantomData,
> +    mem::{self},
> +    ops::Deref,
> +    ptr::{self, NonNull},
>  };
> -use core::{alloc::Layout, mem, ops::Deref, ptr, ptr::NonNull};
>  
>  #[cfg(CONFIG_DRM_LEGACY)]
>  macro_rules! drm_legacy_fields {
> @@ -47,26 +53,88 @@ macro_rules! drm_legacy_fields {
>      }
>  }
>  
> -/// A typed DRM device with a specific `drm::Driver` implementation.
> +macro_rules! drm_dev_ctx {
> +    (
> +        $( #[$attrs:meta] )*
> +        $name:ident
> +    ) => {
> +        $( #[$attrs] )*
> +        pub struct $name;
> +
> +        impl DeviceContext for $name {}
> +        impl Sealed for $name {}
> +
> +        // SAFETY: All registration states are free of side-effects (e.g. no Drop) and are ZSTs,
> +        // thus they are always thread-safe.
> +        unsafe impl Send for $name {}
> +        // SAFETY: All registration states are free of side-effects (e.g. no Drop) and are ZSTs,
> +        // thus they are always thread-safe.
> +        unsafe impl Sync for $name {}

They are all ZSTs and should be Send + Sync by default, hence the impls are not
needed.

Also, I don't think this macro adds a lot of value when the Send and Sync impls
are removed. In the driver core it looks like this:

	mod private {
	    pub trait Sealed {}
	
	    impl Sealed for super::Bound {}
	    impl Sealed for super::Core {}
	    impl Sealed for super::CoreInternal {}
	    impl Sealed for super::Normal {}
	}
	
	impl DeviceContext for Bound {}
	impl DeviceContext for Core {}
	impl DeviceContext for CoreInternal {}
	impl DeviceContext for Normal {}

I think that's compact enough.

> +    };
> +}
> +
> +/// A trait implemented by all possible contexts a [`Device`] can be used in.

I think this documentation should provide a rough overview of the different
states a DRM device can have.

> +pub trait DeviceContext: Sealed + Send + Sync {}
> +
> +drm_dev_ctx! {
> +    /// The [`DeviceContext`] of a [`Device`] that was registered with userspace at some point.
> +    ///
> +    /// This represents a [`Device`] which is guaranteed to have been registered with userspace at
> +    /// some point in time. Such a DRM device is guaranteed to have been fully-initialized.
> +    ///
> +    /// Note: A device in this context is not guaranteed to remain registered with userspace for its
> +    /// entire lifetime, as this is impossible to guarantee at compile-time. However, any
> +    /// userspace-dependent operations performed with an unregistered device in this [`DeviceContext`]
> +    /// are guaranteed to be no-ops.
> +    ///
> +    /// # Invariants
> +    ///
> +    /// A [`Device`] in this [`DeviceContext`] is guaranteed to have called `drm_dev_register` once.

I'm not sure it makes sense for user facing documentation to menthion the
corresponding C function here. I'd just say that it has been registered with the
DRM core.

> +    Registered
> +}
> +
> @@ -132,9 +202,40 @@ pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<A
>  
>          // SAFETY: The reference count is one, and now we take ownership of that reference as a
>          // `drm::Device`.
> -        Ok(unsafe { ARef::from_raw(raw_drm) })
> +        // INVARIANT: We just created the device above, but have yet to call `drm_dev_register`.
> +        // `Self` cannot be copied or sent to another thread - ensuring that `drm_dev_register`
> +        // won't be called during its lifetime and that the device is unregistered

Please end with a period.

> +        Ok(Self(unsafe { ARef::from_raw(raw_drm) }, NotThreadSafe))
>      }
> +}
>  
> +/// A typed DRM device with a specific [`drm::Driver`] implementation and [`DeviceContext`].
> +///
> +/// Since DRM devices can be used before being fully initialized and registered with userspace, `C`
> +/// represents the furthest [`DeviceContext`] we can guarantee that this [`Device`] has reached.
> +///
> +/// Keep in mind: this means that an unregistered device can still have the registration state
> +/// [`Registered`] as long as it was registered with userspace once in the past, and that the
> +/// behavior of such a device is still well-defined. In such a situation, the behavior of any
> +/// functions which interact with userspace will simply be no-ops. Additionally, a device with the

I don't think those are necessarily no-ops, in hot-unplug cases drivers might
still receive DRM callbacks from userspace. But they won't be able to access bus
device resources anymore. In contrast to the DRM C implementation the
corresponding synchronization happens at driver core level in Rust, whereas on
the C side there is drm_dev_enter() / drm_dev_exit() for this.

> +/// registration state [`Uninit`] simply does not have a guaranteed registration state at compile
> +/// time, and could be either registered or unregistered. Since there is no way to guarantee a
> +/// long-lived reference to an unregistered device would remain unregistered, we do not provide a
> +/// [`DeviceContext`] for this.
> +///
> +/// # Invariants
> +///
> +/// * `self.dev` is a valid instance of a `struct device`.
> +/// * The data layout of `Self` remains the same across all implementations of `C`.
> +/// * Any invariants for `C` also apply.
> +#[repr(C)]
> +pub struct Device<T: drm::Driver, C: DeviceContext = Registered> {
> +    dev: Opaque<bindings::drm_device>,
> +    data: T::Data,
> +    _ctx: PhantomData<C>,
> +}
> +
> +impl<T: drm::Driver, C: DeviceContext> Device<T, C> {
>      pub(crate) fn as_raw(&self) -> *mut bindings::drm_device {
>          self.dev.get()
>      }
> @@ -160,13 +261,13 @@ unsafe fn into_drm_device(ptr: NonNull<Self>) -> *mut bindings::drm_device {
>      ///
>      /// # Safety
>      ///
> -    /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count,
> -    /// i.e. it must be ensured that the reference count of the C `struct drm_device` `ptr` points
> -    /// to can't drop to zero, for the duration of this function call and the entire duration when
> -    /// the returned reference exists.
> -    ///
> -    /// Additionally, callers must ensure that the `struct device`, `ptr` is pointing to, is
> -    /// embedded in `Self`.
> +    /// * Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count,
> +    ///   i.e. it must be ensured that the reference count of the C `struct drm_device` `ptr` points
> +    ///   to can't drop to zero, for the duration of this function call and the entire duration when
> +    ///   the returned reference exists.
> +    /// * Additionally, callers must ensure that the `struct device`, `ptr` is pointing to, is

I'd drop "additionally", it is a listing anyways.

> +    ///   embedded in `Self`.
> +    /// * Callers promise that any type invariants of `C` will be upheld.

What do you mean by "type invariants of C"?

>  impl<T: Driver> Registration<T> {
>      /// Creates a new [`Registration`] and registers it.
> -    fn new(drm: &drm::Device<T>, flags: usize) -> Result<Self> {
> +    fn new(drm: drm::UnregisteredDevice<T>, flags: usize) -> Result<Self> {
>          // SAFETY: `drm.as_raw()` is valid by the invariants of `drm::Device`.
>          to_result(unsafe { bindings::drm_dev_register(drm.as_raw(), flags) })?;
>  
> -        Ok(Self(drm.into()))
> +        // SAFETY: We just called `drm_dev_register` above
> +        let new = NonNull::from(unsafe { drm.assume_ctx() });
> +
> +        // Leak the ARef from UnregisteredDevice in preparation for transferring its ownership.
> +        mem::forget(drm);
> +
> +        // SAFETY: `drm`'s `Drop` constructor was never called, ensuring that there remains at least
> +        // one reference to the device - which we take ownership over here.
> +        let new = unsafe { ARef::from_raw(new) };
> +
> +        Ok(Self(new))
>      }
>  
> -    /// Same as [`Registration::new`}, but transfers ownership of the [`Registration`] to
> +    /// Same as [`Registration::new`], but transfers ownership of the [`Registration`] to
>      /// [`devres::register`].
> -    pub fn new_foreign_owned(
> -        drm: &drm::Device<T>,
> -        dev: &device::Device<device::Bound>,
> +    pub fn new_foreign_owned<'a>(
> +        drm: drm::UnregisteredDevice<T>,
> +        dev: &'a device::Device<device::Bound>,
>          flags: usize,
> -    ) -> Result
> +    ) -> Result<&'a drm::Device<T>>
>      where
>          T: 'static,
>      {
> -        if drm.as_ref().as_raw() != dev.as_raw() {
> +        let this_dev: &device::Device = drm.as_ref();
> +        if this_dev.as_raw() != dev.as_raw() {

this_dev is misleading, since it actually is the parent device. Besides that,
the change seems unnecessary, no?

>              return Err(EINVAL);
>          }
>  
>          let reg = Registration::<T>::new(drm, flags)?;
> +        let drm = NonNull::from(reg.device());
> +
> +        devres::register(dev, reg, GFP_KERNEL)?;
>  
> -        devres::register(dev, reg, GFP_KERNEL)
> +        // SAFETY: Since `reg` was passed to devres::register(), the device now owns the lifetime
> +        // of the DRM registration - ensuring that this references lives for at least as long as 'a.
> +        Ok(unsafe { drm.as_ref() })
>      }