Re: [PATCH v5 11/19] rust: drm: add Ioctl device context typestate
"Alexandre Courbot" <[email protected]>
| Newsgroups | dev.linux.lists.nova-gpu,dev.linux.lists.driver-core,org.freedesktop.lists.dri-devel,org.kernel.vger.linux-kernel,org.kernel.vger.rust-for-linux |
|---|---|
| Message-ID | <[email protected]> |
On Sun Jun 28, 2026 at 11:53 PM JST, Danilo Krummrich wrote: > Add the Ioctl DeviceContext for DRM devices that have been registered > with userspace previously. > > A Device<T, Ioctl> has been registered at some point, but may be > concurrently unregistering or already unregistered. drm_dev_enter() can > guard against this, ensuring the device remains registered for the > duration of the critical section. > > This typestate will be used in ioctl dispatch context where registration > is guaranteed by the DRM core, and RegistrationGuard can safely be > acquired. > > Reviewed-by: Lyude Paul <[email protected]> > Signed-off-by: Danilo Krummrich <[email protected]> > --- > rust/kernel/drm/device.rs | 34 +++++++++++++++++++++++++++++++--- > rust/kernel/drm/mod.rs | 1 + > 2 files changed, 32 insertions(+), 3 deletions(-) > > diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs > index 86a7fca1d33f..d429b4655449 100644 > --- a/rust/kernel/drm/device.rs > +++ b/rust/kernel/drm/device.rs > @@ -74,14 +74,16 @@ macro_rules! drm_legacy_fields { > > /// A trait implemented by all possible contexts a [`Device`] can be used in. > /// > -/// A [`Device`] can be in one of two contexts: > +/// A [`Device`] can be in one of the following contexts: > /// > /// - [`Normal`]: The general-purpose, reference-counted context. A [`Device`] in this context may > /// or may not be registered with userspace. > +/// - [`Ioctl`]: The device has been registered with userspace at some point; used in ioctl > +/// dispatch context. > /// - [`Registered`]: The device has been registered with userspace at some point. > /// > -/// `Device<T, Registered>` dereferences to `Device<T>` ([`Normal`]), so any method available on a > -/// [`Normal`] device is also available on a [`Registered`] one. > +/// Both `Device<T, Ioctl>` and `Device<T, Registered>` dereference to `Device<T>` ([`Normal`]), > +/// so any method available on a [`Normal`] device is also available in the other contexts. > pub trait DeviceContext: Sealed + Send + Sync {} > > /// The general-purpose, reference-counted [`DeviceContext`]. > @@ -113,6 +115,21 @@ impl DeviceContext for Normal {} > impl Sealed for Registered {} > impl DeviceContext for Registered {} > > +/// The [`DeviceContext`] of a [`Device`] that has been registered with userspace previously. > +/// > +/// A [`Device`] in this context has been registered at some point, but may be concurrently > +/// unregistering or already unregistered. `drm_dev_enter()` can guard against this, ensuring the > +/// device remains registered for the duration of the critical section. > +/// > +/// # Invariants > +/// > +/// A [`Device`] in this context has been registered with userspace via `drm_dev_register()` at > +/// some point. > +pub struct Ioctl; > + > +impl Sealed for Ioctl {} > +impl DeviceContext for Ioctl {} > + > /// 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, > @@ -342,6 +359,17 @@ fn deref(&self) -> &Self::Target { > } > } > > +impl<T: drm::Driver> Deref for Device<T, Ioctl> { > + type Target = Device<T>; > + > + #[inline] > + fn deref(&self) -> &Self::Target { > + // SAFETY: The caller holds a `Device<T, Ioctl>`, which guarantees all invariants > + // of the weaker `Normal` context. > + unsafe { self.assume_ctx() } > + } > +} Since the `Deref` implementation is the same for `Ioctl` and `Registered`, do we want to add a private marker trait for these two so we can factor out this `impl` block? It wouldn't reduce the LoC count, but would make the logical connection between the two states explicit and set an anchor for potential future blocks with a similar relationship. With or without this: Reviewed-by: Alexandre Courbot <[email protected]>