From: Lyude Paul Date: Thu, 7 May 2026 21:59:19 +0000 (-0400) Subject: rust/drm: Introduce DeviceContext X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=571d4242c466e558c9fd57a9b3d9a045b1f400d1;p=thirdparty%2Flinux.git rust/drm: Introduce DeviceContext One of the tricky things about DRM bindings in Rust is the fact that initialization of a DRM device is a multi-step process. It's quite normal for a device driver to start making use of its DRM device for tasks like creating GEM objects before userspace registration happens. This is an issue in rust though, since prior to userspace registration the device is only partly initialized. This means there's a plethora of DRM device operations we can't yet expose without opening up the door to UB if the DRM device in question isn't yet registered. Additionally, this isn't something we can reliably check at runtime. And even if we could, performing an operation which requires the device be registered when the device isn't actually registered is a programmer bug, meaning there's no real way to gracefully handle such a mistake at runtime. And even if that wasn't the case, it would be horrendously annoying and noisy to have to check if a device is registered constantly throughout a driver. In order to solve this, we first take inspiration from `kernel::device::DeviceContext` and introduce `kernel::drm::DeviceContext`. This provides us with a ZST type that we can generalize over to represent contexts where a device is known to have been registered with userspace at some point in time (`Registered`), along with contexts where we can't make such a guarantee (`Uninit`). It's important to note we intentionally do not provide a `DeviceContext` which represents an unregistered device. This is because there's no reasonable way to guarantee that a device with long-living references to itself will not be registered eventually with userspace. Instead, we provide a new-type for this: `UnregisteredDevice` which can provide a guarantee that the `Device` has never been registered with userspace. To ensure this, we modify `Registration` so that creating a new `Registration` requires passing ownership of an `UnregisteredDevice`. Signed-off-by: Lyude Paul Reviewed-by: Daniel Almeida Link: https://patch.msgid.link/20260507220044.3204919-2-lyude@redhat.com Signed-off-by: Danilo Krummrich --- diff --git a/drivers/gpu/drm/nova/driver.rs b/drivers/gpu/drm/nova/driver.rs index 4289df7de01cd..33fe088dd3b9a 100644 --- a/drivers/gpu/drm/nova/driver.rs +++ b/drivers/gpu/drm/nova/driver.rs @@ -23,7 +23,7 @@ pub(crate) struct Nova { } /// Convienence type alias for the DRM device type for this driver -pub(crate) type NovaDevice = drm::Device; +pub(crate) type NovaDevice = drm::Device; #[pin_data] pub(crate) struct NovaData { @@ -62,10 +62,10 @@ impl auxiliary::Driver for NovaDriver { ) -> impl PinInit, Error> + 'bound { let data = try_pin_init!(NovaData { adev: adev.into() }); - let drm = drm::Device::::new(adev.as_ref(), data)?; - drm::Registration::new_foreign_owned(&drm, adev.as_ref(), 0)?; + let drm = drm::UnregisteredDevice::::new(adev.as_ref(), data)?; + let drm = drm::Registration::new_foreign_owned(drm, adev.as_ref(), 0)?; - Ok(Nova { drm }) + Ok(Nova { drm: drm.into() }) } } diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs index 227ea2adcceaf..3e53ecd331433 100644 --- a/drivers/gpu/drm/tyr/driver.rs +++ b/drivers/gpu/drm/tyr/driver.rs @@ -46,7 +46,7 @@ pub(crate) type IoMem<'a> = kernel::io::mem::IoMem<'a, SZ_2M>; pub(crate) struct TyrDrmDriver; /// Convenience type alias for the DRM device type for this driver. -pub(crate) type TyrDrmDevice = drm::Device; +pub(crate) type TyrDrmDevice = drm::Device; pub(crate) struct TyrPlatformDriver; @@ -148,10 +148,12 @@ impl platform::Driver for TyrPlatformDriver { gpu_info, }); - let ddev: ARef = drm::Device::new(pdev.as_ref(), data)?; - drm::driver::Registration::new_foreign_owned(&ddev, pdev.as_ref(), 0)?; + let tdev = drm::UnregisteredDevice::::new(pdev.as_ref(), data)?; + let tdev = drm::driver::Registration::new_foreign_owned(tdev, pdev.as_ref(), 0)?; - let driver = TyrPlatformDriverData { _device: ddev }; + let driver = TyrPlatformDriverData { + _device: tdev.into(), + }; // We need this to be dev_info!() because dev_dbg!() does not work at // all in Rust for now, and we need to see whether probe succeeded. diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs index dc16738eeb38a..a47d81a267244 100644 --- a/rust/kernel/drm/device.rs +++ b/rust/kernel/drm/device.rs @@ -6,10 +6,12 @@ use crate::{ alloc::allocator::Kmalloc, - bindings, device, + bindings, + device, drm::{ self, - driver::AllocImpl, // + driver::AllocImpl, + private::Sealed, // }, error::from_err_ptr, prelude::*, @@ -17,16 +19,20 @@ use crate::{ ARef, AlwaysRefCounted, // }, - types::Opaque, + types::{ + NotThreadSafe, + Opaque, // + }, workqueue::{ HasDelayedWork, HasWork, Work, WorkItem, // - }, + }, // }; use core::{ alloc::Layout, + marker::PhantomData, mem, ops::Deref, ptr::{ @@ -66,20 +72,92 @@ macro_rules! drm_legacy_fields { } } -/// A typed DRM device with a specific `drm::Driver` implementation. +/// A trait implemented by all possible contexts a [`Device`] can be used in. +/// +/// Setting up a new [`Device`] is a multi-stage process. Each step of the process that a user +/// interacts with in Rust has a respective [`DeviceContext`] typestate. For example, +/// `Device` would be a [`Device`] that reached the [`Registered`] [`DeviceContext`]. +/// +/// Each stage of this process is described below: +/// +/// ```text +/// 1 2 3 +/// +--------------+ +------------------+ +-----------------------+ +/// |Device created| → |Device initialized| → |Registered w/ userspace| +/// +--------------+ +------------------+ +-----------------------+ +/// (Uninit) (Registered) +/// ``` +/// +/// 1. The [`Device`] is in the [`Uninit`] context and is not guaranteed to be initialized or +/// registered with userspace. Only a limited subset of DRM core functionality is available. +/// 2. The [`Device`] is guaranteed to be fully initialized, but is not guaranteed to be registered +/// with userspace. All DRM core functionality which doesn't interact with userspace is +/// available. We currently don't have a context for representing this. +/// 3. The [`Device`] is guaranteed to be fully initialized, and is guaranteed to have been +/// registered with userspace at some point - thus putting it in the [`Registered`] context. /// -/// The device is always reference-counted. +/// An important caveat of [`DeviceContext`] which must be kept in mind: when used as a typestate +/// for a reference type, it can only guarantee that a [`Device`] reached a particular stage in the +/// initialization process _at the time the reference was taken_. No guarantee is made in regards to +/// what stage of the process the [`Device`] is currently in. This means for instance that a +/// `&Device` may actually be registered with userspace, it just wasn't known to be +/// registered at the time the reference was taken. +pub trait DeviceContext: Sealed + Send + Sync {} + +/// 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. /// /// # Invariants /// -/// `self.dev` is a valid instance of a `struct device`. -#[repr(C)] -pub struct Device { - dev: Opaque, - data: T::Data, +/// A [`Device`] in this [`DeviceContext`] is guaranteed to have been registered with userspace +/// at some point in time. +pub struct Registered; + +impl Sealed for Registered {} +impl DeviceContext for Registered {} + +/// The [`DeviceContext`] of a [`Device`] that may be unregistered and partly uninitialized. +/// +/// A [`Device`] in this context is only guaranteed to be partly initialized, and may or may not +/// be registered with userspace. Thus operations which depend on the [`Device`] being fully +/// initialized, or which depend on the [`Device`] being registered with userspace are not +/// available through this [`DeviceContext`]. +/// +/// A [`Device`] in this context can be used to create a +/// [`Registration`](drm::driver::Registration). +pub struct Uninit; + +impl Sealed for Uninit {} +impl DeviceContext for Uninit {} + +/// A [`Device`] which is known at compile-time to be unregistered with userspace. +/// +/// This type allows performing operations which are only safe to do before userspace registration, +/// and can be used to create a [`Registration`](drm::driver::Registration) once the driver is ready +/// to register the device with userspace. +/// +/// Since DRM device initialization must be single-threaded, this object is not thread-safe. +/// +/// # Invariants +/// +/// The device in `self.0` is guaranteed to be a newly created [`Device`] that has not yet been +/// registered with userspace until this type is dropped. +pub struct UnregisteredDevice(ARef>, NotThreadSafe); + +impl Deref for UnregisteredDevice { + type Target = Device; + + fn deref(&self) -> &Self::Target { + &self.0 + } } -impl Device { +impl UnregisteredDevice { const fn compute_features() -> u32 { let mut features = drm::driver::FEAT_GEM; @@ -95,7 +173,7 @@ impl Device { open: Some(drm::File::::open_callback), postclose: Some(drm::File::::postclose_callback), unload: None, - release: Some(Self::release), + release: Some(Device::::release), master_set: None, master_drop: None, debugfs_init: None, @@ -123,11 +201,13 @@ impl Device { const GEM_FOPS: bindings::file_operations = drm::gem::create_fops(); - /// Create a new `drm::Device` for a `drm::Driver`. - pub fn new(dev: &device::Device, data: impl PinInit) -> Result> { + /// Create a new `UnregisteredDevice` for a `drm::Driver`. + /// + /// This can be used to create a [`Registration`](kernel::drm::Registration). + pub fn new(dev: &device::Device, data: impl PinInit) -> Result { // `__drm_dev_alloc` uses `kmalloc()` to allocate memory, hence ensure a `kmalloc()` // compatible `Layout`. - let layout = Kmalloc::aligned_layout(Layout::new::()); + let layout = Kmalloc::aligned_layout(Layout::new::>()); // Use a temporary vtable without a `release` callback until `data` is initialized, so // init failure can release the DRM device without dropping uninitialized fields. @@ -139,12 +219,12 @@ impl Device { // SAFETY: // - `alloc_vtable` reference remains valid until no longer used, // - `dev` is valid by its type invarants, - let raw_drm: *mut Self = unsafe { + let raw_drm: *mut Device = unsafe { bindings::__drm_dev_alloc( dev.as_raw(), &alloc_vtable, layout.size(), - mem::offset_of!(Self, dev), + mem::offset_of!(Device, dev), ) } .cast(); @@ -152,7 +232,7 @@ impl Device { // SAFETY: `raw_drm` is a valid pointer to `Self`, given that `__drm_dev_alloc` was // successful. - let drm_dev = unsafe { Self::into_drm_device(raw_drm) }; + let drm_dev = unsafe { Device::into_drm_device(raw_drm) }; // SAFETY: `raw_drm` is a valid pointer to `Self`. let raw_data = unsafe { ptr::addr_of_mut!((*raw_drm.as_ptr()).data) }; @@ -171,9 +251,39 @@ impl Device { // 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. + 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. Additionally, a device with the 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 { + dev: Opaque, + data: T::Data, + _ctx: PhantomData, +} +impl Device { pub(crate) fn as_raw(&self) -> *mut bindings::drm_device { self.dev.get() } @@ -199,13 +309,13 @@ impl 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 + /// embedded in `Self`. + /// * Callers promise that any type invariants of `C` will be upheld. #[doc(hidden)] pub unsafe fn from_raw<'a>(ptr: *const bindings::drm_device) -> &'a Self { // SAFETY: By the safety requirements of this function `ptr` is a valid pointer to a @@ -225,9 +335,20 @@ impl Device { // - `this` is valid for dropping. unsafe { core::ptr::drop_in_place(this) }; } + + /// Change the [`DeviceContext`] for a [`Device`]. + /// + /// # Safety + /// + /// The caller promises that `self` fulfills all of the guarantees provided by the given + /// [`DeviceContext`]. + pub(crate) unsafe fn assume_ctx(&self) -> &Device { + // SAFETY: The data layout is identical via our type invariants. + unsafe { mem::transmute(self) } + } } -impl Deref for Device { +impl Deref for Device { type Target = T::Data; fn deref(&self) -> &Self::Target { @@ -237,7 +358,7 @@ impl Deref for Device { // SAFETY: DRM device objects are always reference counted and the get/put functions // satisfy the requirements. -unsafe impl AlwaysRefCounted for Device { +unsafe impl AlwaysRefCounted for Device { fn inc_ref(&self) { // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero. unsafe { bindings::drm_dev_get(self.as_raw()) }; @@ -252,7 +373,7 @@ unsafe impl AlwaysRefCounted for Device { } } -impl AsRef for Device { +impl AsRef for Device { fn as_ref(&self) -> &device::Device { // SAFETY: `bindings::drm_device::dev` is valid as long as the DRM device itself is valid, // which is guaranteed by the type invariant. @@ -261,21 +382,22 @@ impl AsRef for Device { } // SAFETY: A `drm::Device` can be released from any thread. -unsafe impl Send for Device {} +unsafe impl Send for Device {} // SAFETY: A `drm::Device` can be shared among threads because all immutable methods are protected // by the synchronization in `struct drm_device`. -unsafe impl Sync for Device {} +unsafe impl Sync for Device {} -impl WorkItem for Device +impl WorkItem for Device where T: drm::Driver, - T::Data: WorkItem>>, - T::Data: HasWork, ID>, + T::Data: WorkItem>, + T::Data: HasWork, + C: DeviceContext, { - type Pointer = ARef>; + type Pointer = ARef; - fn run(ptr: ARef>) { + fn run(ptr: ARef) { T::Data::run(ptr); } } @@ -287,40 +409,42 @@ where // stored inline in `drm::Device`, so the `container_of` call is valid. // // - The two methods are true inverses of each other: given `ptr: *mut -// Device`, `raw_get_work` will return a `*mut Work, ID>` through -// `T::Data::raw_get_work` and given a `ptr: *mut Work, ID>`, -// `work_container_of` will return a `*mut Device` through `container_of`. -unsafe impl HasWork, ID> for Device +// Device`, `raw_get_work` will return a `*mut Work, ID>` through +// `T::Data::raw_get_work` and given a `ptr: *mut Work, ID>`, +// `work_container_of` will return a `*mut Device` through `container_of`. +unsafe impl HasWork for Device where T: drm::Driver, - T::Data: HasWork, ID>, + T::Data: HasWork, + C: DeviceContext, { - unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work, ID> { - // SAFETY: The caller promises that `ptr` points to a valid `Device`. + unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work { + // SAFETY: The caller promises that `ptr` points to a valid `Device`. let data_ptr = unsafe { &raw mut (*ptr).data }; // SAFETY: `data_ptr` is a valid pointer to `T::Data`. unsafe { T::Data::raw_get_work(data_ptr) } } - unsafe fn work_container_of(ptr: *mut Work, ID>) -> *mut Self { + unsafe fn work_container_of(ptr: *mut Work) -> *mut Self { // SAFETY: The caller promises that `ptr` points at a `Work` field in // `T::Data`. let data_ptr = unsafe { T::Data::work_container_of(ptr) }; - // SAFETY: `T::Data` is stored as the `data` field in `Device`. + // SAFETY: `T::Data` is stored as the `data` field in `Device`. unsafe { crate::container_of!(data_ptr, Self, data) } } } // SAFETY: Our `HasWork` implementation returns a `work_struct` that is // stored in the `work` field of a `delayed_work` with the same access rules as -// the `work_struct` owing to the bound on `T::Data: HasDelayedWork, +// the `work_struct` owing to the bound on `T::Data: HasDelayedWork, // ID>`, which requires that `T::Data::raw_get_work` return a `work_struct` that // is inside a `delayed_work`. -unsafe impl HasDelayedWork, ID> for Device +unsafe impl HasDelayedWork for Device where T: drm::Driver, - T::Data: HasDelayedWork, ID>, + T::Data: HasDelayedWork, + C: DeviceContext, { } diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs index 6886396c8fa78..07af409959a9a 100644 --- a/rust/kernel/drm/driver.rs +++ b/rust/kernel/drm/driver.rs @@ -13,6 +13,10 @@ use crate::{ prelude::*, sync::aref::ARef, // }; +use core::{ + mem, + ptr::NonNull, // +}; /// Driver use the GEM memory manager. This should be set for all modern drivers. pub(crate) const FEAT_GEM: u32 = bindings::drm_driver_feature_DRIVER_GEM; @@ -135,21 +139,31 @@ pub trait Driver { pub struct Registration(ARef>); impl Registration { - fn new(drm: &drm::Device, flags: usize) -> Result { + fn new(drm: drm::UnregisteredDevice, flags: usize) -> Result { // 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)) } - /// 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, - dev: &device::Device, + pub fn new_foreign_owned<'a>( + drm: drm::UnregisteredDevice, + dev: &'a device::Device, flags: usize, - ) -> Result + ) -> Result<&'a drm::Device> where T: 'static, { @@ -158,8 +172,13 @@ impl Registration { } let reg = Registration::::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() }) } /// Returns a reference to the `Device` instance for this registration. diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs index a4b6c54301985..a66e7166f66b5 100644 --- a/rust/kernel/drm/mod.rs +++ b/rust/kernel/drm/mod.rs @@ -10,6 +10,10 @@ pub mod gpuvm; pub mod ioctl; pub use self::device::Device; +pub use self::device::DeviceContext; +pub use self::device::Registered; +pub use self::device::Uninit; +pub use self::device::UnregisteredDevice; pub use self::driver::Driver; pub use self::driver::DriverInfo; pub use self::driver::Registration;