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() })
> }
