diff --git a/drivers/gpu/drm/nova/driver.rs b/drivers/gpu/drm/nova/driver.rs index 4289df7de01c..33fe088dd3b9 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 @@ fn probe<'bound>( ) -> 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 227ea2adccea..3e53ecd33143 100644 --- a/drivers/gpu/drm/tyr/driver.rs +++ b/drivers/gpu/drm/tyr/driver.rs @@ -46,7 +46,7 @@ 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 @@ fn probe<'bound>( 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 dc16738eeb38..a47d81a26724 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 @@ 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. /// -/// The device is always reference-counted. +/// 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. +/// +/// 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 @@ const fn compute_features() -> u32 { 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 @@ const fn compute_features() -> u32 { 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 @@ pub fn new(dev: &device::Device, data: impl PinInit) -> Result = 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 @@ pub fn new(dev: &device::Device, data: impl PinInit) -> Result) -> Result { + 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 @@ unsafe fn into_drm_device(ptr: NonNull) -> *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 + /// 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 @@ extern "C" fn release(ptr: *mut bindings::drm_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 @@ fn deref(&self) -> &Self::Target { // 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 fn dec_ref(obj: NonNull) { } } -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 @@ fn as_ref(&self) -> &device::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 @@ fn run(ptr: ARef>) { // 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 6886396c8fa7..07af409959a9 100644 --- a/rust/kernel/drm/driver.rs +++ b/rust/kernel/drm/driver.rs @@ -13,6 +13,10 @@ 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 @@ pub fn new_foreign_owned( } 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 a4b6c5430198..a66e7166f66b 100644 --- a/rust/kernel/drm/mod.rs +++ b/rust/kernel/drm/mod.rs @@ -10,6 +10,10 @@ 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;