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

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

On Thu Jan 22, 2026 at 11:46 PM CET, Lyude Paul wrote:
> diff --git a/drivers/gpu/drm/nova/driver.rs b/drivers/gpu/drm/nova/driver.rs
> index b1af0a099551d..99d6841b69cbc 100644
> --- a/drivers/gpu/drm/nova/driver.rs
> +++ b/drivers/gpu/drm/nova/driver.rs
> @@ -21,7 +21,7 @@ pub(crate) struct NovaDriver {
>  }
>  
>  /// Convienence type alias for the DRM device type for this driver
> -pub(crate) type NovaDevice = drm::Device<NovaDriver>;
> +pub(crate) type NovaDevice<Ctx = drm::Registered> = drm::Device<NovaDriver, Ctx>;

Nothing for this patch (series), but I think we should get rid of this type
alias, I think it's confusing.

> +/// 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

This is still not correct, the are not guaranteed to be no-ops. We can still
have callbacks from userspace after the DRM device is unregistered.

> +/// 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>,
> +}

<snip>

> -    /// Registers a new [`Device`](drm::Device) with userspace.
> +    /// Registers a new [`UnregisteredDevice`](drm::UnregisteredDevice) with userspace.
>      ///
>      /// Ownership of the [`Registration`] object is passed 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() {

I still think this change is unnecessary and the name 'this_dev' is misleading,
as it actually is the parent device.

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