| Message ID | 20250325235522.3992-6-dakr@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | DRM Rust abstractions | expand |
Hi, On Wed, Mar 26, 2025 at 12:54:32AM +0100, Danilo Krummrich wrote: > Implement the DRM driver `Registration`. > > The `Registration` structure is responsible to register and unregister a > DRM driver. It makes use of the `Devres` container in order to allow the > `Registration` to be owned by devres, such that it is automatically > dropped (and the DRM driver unregistered) once the parent device is > unbound. The code looks correct, but the wording is confusing to me. drm_dev_unregister does indeed unregister the device, but it's not freed until the last reference is dropped, so it's not really "dropped once the parent device is unbound", the reference is, and it's not active anymore. > Co-developed-by: Asahi Lina <lina@asahilina.net> > Signed-off-by: Asahi Lina <lina@asahilina.net> > Signed-off-by: Danilo Krummrich <dakr@kernel.org> > --- > rust/kernel/drm/driver.rs | 57 ++++++++++++++++++++++++++++++++++++++- > rust/kernel/drm/mod.rs | 1 + > 2 files changed, 57 insertions(+), 1 deletion(-) > > diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs > index 625cab7839a3..8d2b397018d1 100644 > --- a/rust/kernel/drm/driver.rs > +++ b/rust/kernel/drm/driver.rs > @@ -4,7 +4,15 @@ > //! > //! C header: [`include/linux/drm/drm_drv.h`](srctree/include/linux/drm/drm_drv.h) > > -use crate::{bindings, drm, str::CStr}; > +use crate::{ > + bindings, > + devres::Devres, > + drm, > + error::{Error, Result}, > + prelude::*, > + str::CStr, > + types::ARef, > +}; > use macros::vtable; > > /// Driver use the GEM memory manager. This should be set for all modern drivers. > @@ -134,3 +142,50 @@ pub trait Driver { > /// IOCTL list. See `kernel::drm::ioctl::declare_drm_ioctls!{}`. > const IOCTLS: &'static [drm::ioctl::DrmIoctlDescriptor]; > } > + > +/// The registration type of a `drm::Device`. > +/// > +/// Once the `Registration` structure is dropped, the device is unregistered. > +pub struct Registration<T: Driver>(ARef<drm::Device<T>>); > + > +impl<T: Driver> Registration<T> { > + /// Creates a new [`Registration`] and registers it. > + pub fn new(drm: ARef<drm::Device<T>>, flags: usize) -> Result<Self> { > + // SAFETY: Safe by the invariants of `drm::Device`. > + let ret = unsafe { bindings::drm_dev_register(drm.as_raw(), flags) }; > + if ret < 0 { > + return Err(Error::from_errno(ret)); > + } > + > + Ok(Self(drm)) > + } > + > + /// Same as [`Registration::new`}, but transfers ownership of the [`Registration`] to > + /// [`Devres`]. > + pub fn new_foreign_owned(drm: ARef<drm::device::Device<T>>, flags: usize) -> Result { > + let reg = Registration::<T>::new(drm.clone(), flags)?; > + > + Devres::new_foreign_owned(drm.as_ref(), reg, GFP_KERNEL) > + } > + > + /// Returns a reference to the `Device` instance for this registration. > + pub fn device(&self) -> &drm::Device<T> { > + &self.0 > + } > +} > + > +// SAFETY: `Registration` doesn't offer any methods or access to fields when shared between > +// threads, hence it's safe to share it. > +unsafe impl<T: Driver> Sync for Registration<T> {} > + > +// SAFETY: Registration with and unregistration from the DRM subsystem can happen from any thread. > +unsafe impl<T: Driver> Send for Registration<T> {} > + > +impl<T: Driver> Drop for Registration<T> { > + /// Removes the registration from the kernel if it has completed successfully before. > + fn drop(&mut self) { > + // SAFETY: Safe by the invariant of `ARef<drm::Device<T>>`. The existence of this > + // `Registration` also guarantees the this `drm::Device` is actually registered. > + unsafe { bindings::drm_dev_unregister(self.0.as_raw()) }; > + } > +} drm_dev_unregister also have an hotplug-aware variant in drm_dev_unplug(). However, most devices are hotpluggable, even if only through sysfs. So drm_dev_unplug() is generally a better option. Should we use it here, and / or should we provide multiple options still? Another thing worth mentioning I think is that drm_dev_unplug() works by setting a flag, and drivers are expected to check that their access to device-bound resources (so registers mapping, clocks, regulators, etc.) are still there through drm_dev_enter/drm_dev_exit. It's pretty fragile overall, so I wonder if it's something we could abstract away in Rust. Maxime
On Wed, Mar 26, 2025 at 10:24:43AM +0100, Maxime Ripard wrote: > Hi, > > On Wed, Mar 26, 2025 at 12:54:32AM +0100, Danilo Krummrich wrote: > > Implement the DRM driver `Registration`. > > > > The `Registration` structure is responsible to register and unregister a > > DRM driver. It makes use of the `Devres` container in order to allow the > > `Registration` to be owned by devres, such that it is automatically > > dropped (and the DRM driver unregistered) once the parent device is > > unbound. > > The code looks correct, but the wording is confusing to me. > drm_dev_unregister does indeed unregister the device, but it's not freed > until the last reference is dropped, so it's not really "dropped once > the parent device is unbound", the reference is, and it's not active > anymore. The above wording is related to the Registration structure itself, i.e. the Registration is dropped, but not the the DRM device itself. However, if the Registration had the last reference to the DRM device, then of course it's freed. > > +impl<T: Driver> Drop for Registration<T> { > > + /// Removes the registration from the kernel if it has completed successfully before. > > + fn drop(&mut self) { > > + // SAFETY: Safe by the invariant of `ARef<drm::Device<T>>`. The existence of this > > + // `Registration` also guarantees the this `drm::Device` is actually registered. > > + unsafe { bindings::drm_dev_unregister(self.0.as_raw()) }; > > + } > > +} > > drm_dev_unregister also have an hotplug-aware variant in > drm_dev_unplug(). However, most devices are hotpluggable, even if only > through sysfs. So drm_dev_unplug() is generally a better option. Should > we use it here, and / or should we provide multiple options still? > > Another thing worth mentioning I think is that drm_dev_unplug() works by > setting a flag, and drivers are expected to check that their access to > device-bound resources (so registers mapping, clocks, regulators, etc.) > are still there through drm_dev_enter/drm_dev_exit. It's pretty fragile > overall, so I wonder if it's something we could abstract away in Rust. DRM should not have to take care of the lifetime of device resources of the parent device. This is the responsibility of the driver core abstractions. At least for the device resources we directly give out to drivers, it has to be, since otherwise it would mean that the driver core abstraction is unsound. Drivers could otherwise extend the lifetime of device resources arbitrarily. For instance, I/O memory is only ever given out by bus abstractions embedded in a Devres container (e.g. Devres<pci::Bar>). Once the parent device is unbound the pci::Bar within the Devres container won't be accessible any more and will be dropped internally. So, that's nothing DRM has to take care of. However, there are cases where it's better to let subsystem abstractions manage the lifetime of device resources, (e.g. DMA mappings). The relevant thing is, that we never hand out device resources to a driver in a way that the driver can extend their lifetime arbitrarily. There are also other mechanisms that I plan to encode in the type system of (bus) devices. With [1] I implemented a generic for (bus specific) devices to indicate their context (e.g. to indicate whether this reference comes from a bus callback, in which case we'd be allowed to call some methods without (additional) synchronization). I want to use this to also let abstractions indicate whether the device is guaranteed to be bound through the entire duration of subsequent calls to drivers. So, there are basically three ways to deal with this: 1. Use type system encodings where possible, since it can be validated at compile time and is zero cost on runtime. 2. Let specific subsystem abstractions take care of device resource lifetimes. 3. Wrap device resources directly managed by drivers in a Devres container. Also note that for Devres, I'm working on patches that will ensure that there is only one single synchronize_rcu() per device / driver binding, rather than for every Devres container instance. [1] https://lore.kernel.org/lkml/20250314160932.100165-1-dakr@kernel.org/
On Wed, Mar 26, 2025 at 11:46:54AM +0100, Danilo Krummrich wrote: > On Wed, Mar 26, 2025 at 10:24:43AM +0100, Maxime Ripard wrote: > > Hi, > > > > On Wed, Mar 26, 2025 at 12:54:32AM +0100, Danilo Krummrich wrote: > > > Implement the DRM driver `Registration`. > > > > > > The `Registration` structure is responsible to register and unregister a > > > DRM driver. It makes use of the `Devres` container in order to allow the > > > `Registration` to be owned by devres, such that it is automatically > > > dropped (and the DRM driver unregistered) once the parent device is > > > unbound. > > > > The code looks correct, but the wording is confusing to me. > > drm_dev_unregister does indeed unregister the device, but it's not freed > > until the last reference is dropped, so it's not really "dropped once > > the parent device is unbound", the reference is, and it's not active > > anymore. > > The above wording is related to the Registration structure itself, i.e. the > Registration is dropped, but not the the DRM device itself. However, if the > Registration had the last reference to the DRM device, then of course it's > freed. Ok, my bad then :) > > > +impl<T: Driver> Drop for Registration<T> { > > > + /// Removes the registration from the kernel if it has completed successfully before. > > > + fn drop(&mut self) { > > > + // SAFETY: Safe by the invariant of `ARef<drm::Device<T>>`. The existence of this > > > + // `Registration` also guarantees the this `drm::Device` is actually registered. > > > + unsafe { bindings::drm_dev_unregister(self.0.as_raw()) }; > > > + } > > > +} > > > > drm_dev_unregister also have an hotplug-aware variant in > > drm_dev_unplug(). However, most devices are hotpluggable, even if only > > through sysfs. So drm_dev_unplug() is generally a better option. Should > > we use it here, and / or should we provide multiple options still? > > > > Another thing worth mentioning I think is that drm_dev_unplug() works by > > setting a flag, and drivers are expected to check that their access to > > device-bound resources (so registers mapping, clocks, regulators, etc.) > > are still there through drm_dev_enter/drm_dev_exit. It's pretty fragile > > overall, so I wonder if it's something we could abstract away in Rust. > > DRM should not have to take care of the lifetime of device resources of the > parent device. This is the responsibility of the driver core abstractions. > > At least for the device resources we directly give out to drivers, it has to be, > since otherwise it would mean that the driver core abstraction is unsound. > Drivers could otherwise extend the lifetime of device resources arbitrarily. Sure. > For instance, I/O memory is only ever given out by bus abstractions embedded in > a Devres container (e.g. Devres<pci::Bar>). Once the parent device is unbound > the pci::Bar within the Devres container won't be accessible any more and will > be dropped internally. So, that's nothing DRM has to take care of. > > However, there are cases where it's better to let subsystem abstractions manage > the lifetime of device resources, (e.g. DMA mappings). The relevant thing is, > that we never hand out device resources to a driver in a way that the driver can > extend their lifetime arbitrarily. I was talking about the opposite though: when the driver is still around but the device (and its resources) aren't anymore. It makes total sense to tie the lifetime of the device resources to the device. However, the DRM device and driver will far outlive the device it was bound to so it needs to deal with that kind of degraded "the DRM driver can still be called by userspace, but it doesn't have a device anymore" scenario. That's what I was talking about. Maxime
On Fri, Mar 28, 2025 at 03:28:04PM +0100, Maxime Ripard wrote: > On Wed, Mar 26, 2025 at 11:46:54AM +0100, Danilo Krummrich wrote: > > On Wed, Mar 26, 2025 at 10:24:43AM +0100, Maxime Ripard wrote: > > > On Wed, Mar 26, 2025 at 12:54:32AM +0100, Danilo Krummrich wrote: > > > > drm_dev_unregister also have an hotplug-aware variant in > > > drm_dev_unplug(). However, most devices are hotpluggable, even if only > > > through sysfs. So drm_dev_unplug() is generally a better option. Should > > > we use it here, and / or should we provide multiple options still? > > > > > > Another thing worth mentioning I think is that drm_dev_unplug() works by > > > setting a flag, and drivers are expected to check that their access to > > > device-bound resources (so registers mapping, clocks, regulators, etc.) > > > are still there through drm_dev_enter/drm_dev_exit. It's pretty fragile > > > overall, so I wonder if it's something we could abstract away in Rust. > > > > DRM should not have to take care of the lifetime of device resources of the > > parent device. This is the responsibility of the driver core abstractions. > > > > At least for the device resources we directly give out to drivers, it has to be, > > since otherwise it would mean that the driver core abstraction is unsound. > > Drivers could otherwise extend the lifetime of device resources arbitrarily. > > Sure. > > > For instance, I/O memory is only ever given out by bus abstractions embedded in > > a Devres container (e.g. Devres<pci::Bar>). Once the parent device is unbound > > the pci::Bar within the Devres container won't be accessible any more and will > > be dropped internally. So, that's nothing DRM has to take care of. > > > > However, there are cases where it's better to let subsystem abstractions manage > > the lifetime of device resources, (e.g. DMA mappings). The relevant thing is, > > that we never hand out device resources to a driver in a way that the driver can > > extend their lifetime arbitrarily. > > I was talking about the opposite though: when the driver is still around > but the device (and its resources) aren't anymore. Well, that's what I was talking about too. :) > It makes total sense to tie the lifetime of the device resources to the > device. However, the DRM device and driver will far outlive the device > it was bound to so it needs to deal with that kind of degraded "the DRM > driver can still be called by userspace, but it doesn't have a device > anymore" scenario. That's what I was talking about. This scenario should be covered by the things I mentioned above. Let's take the I/O memory example. If you call into the DRM driver from userspace when the underlying bus device has already been unbound, the DRM driver may still hold a Devres<pci::Bar> instance. If the DRM driver then calls bar.try_access() (in order to subsequently call writeX() or readX()) the try_access() call will fail, since the corresponding PCI device has been unbound already. In other words the pci::Bar instance within the Devres container will be dropped (which includes unmapping the bar and releasing the resource region) when the PCI device is unbound internally and the Devres container will restrict subsequent accesses from drivers. It pretty much does the same thing as drm_dev_enter() / drm_dev_exit(), but additionally prevents access to the (meanwhile) invalid pointer to the device resource and ensures that the driver can't make device resources out-live device unbind. As mentioned above, the Devres container is just one example of how we prevent such things; it depends on the exact scenario. In either case, I don't want the driver itself to be responsible to setup the corresponding guards, that would make the corresponding abstractions unsound.
diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs index 625cab7839a3..8d2b397018d1 100644 --- a/rust/kernel/drm/driver.rs +++ b/rust/kernel/drm/driver.rs @@ -4,7 +4,15 @@ //! //! C header: [`include/linux/drm/drm_drv.h`](srctree/include/linux/drm/drm_drv.h) -use crate::{bindings, drm, str::CStr}; +use crate::{ + bindings, + devres::Devres, + drm, + error::{Error, Result}, + prelude::*, + str::CStr, + types::ARef, +}; use macros::vtable; /// Driver use the GEM memory manager. This should be set for all modern drivers. @@ -134,3 +142,50 @@ pub trait Driver { /// IOCTL list. See `kernel::drm::ioctl::declare_drm_ioctls!{}`. const IOCTLS: &'static [drm::ioctl::DrmIoctlDescriptor]; } + +/// The registration type of a `drm::Device`. +/// +/// Once the `Registration` structure is dropped, the device is unregistered. +pub struct Registration<T: Driver>(ARef<drm::Device<T>>); + +impl<T: Driver> Registration<T> { + /// Creates a new [`Registration`] and registers it. + pub fn new(drm: ARef<drm::Device<T>>, flags: usize) -> Result<Self> { + // SAFETY: Safe by the invariants of `drm::Device`. + let ret = unsafe { bindings::drm_dev_register(drm.as_raw(), flags) }; + if ret < 0 { + return Err(Error::from_errno(ret)); + } + + Ok(Self(drm)) + } + + /// Same as [`Registration::new`}, but transfers ownership of the [`Registration`] to + /// [`Devres`]. + pub fn new_foreign_owned(drm: ARef<drm::device::Device<T>>, flags: usize) -> Result { + let reg = Registration::<T>::new(drm.clone(), flags)?; + + Devres::new_foreign_owned(drm.as_ref(), reg, GFP_KERNEL) + } + + /// Returns a reference to the `Device` instance for this registration. + pub fn device(&self) -> &drm::Device<T> { + &self.0 + } +} + +// SAFETY: `Registration` doesn't offer any methods or access to fields when shared between +// threads, hence it's safe to share it. +unsafe impl<T: Driver> Sync for Registration<T> {} + +// SAFETY: Registration with and unregistration from the DRM subsystem can happen from any thread. +unsafe impl<T: Driver> Send for Registration<T> {} + +impl<T: Driver> Drop for Registration<T> { + /// Removes the registration from the kernel if it has completed successfully before. + fn drop(&mut self) { + // SAFETY: Safe by the invariant of `ARef<drm::Device<T>>`. The existence of this + // `Registration` also guarantees the this `drm::Device` is actually registered. + unsafe { bindings::drm_dev_unregister(self.0.as_raw()) }; + } +} diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs index 967854a2083e..2d88e70ba607 100644 --- a/rust/kernel/drm/mod.rs +++ b/rust/kernel/drm/mod.rs @@ -9,6 +9,7 @@ pub use self::device::Device; pub use self::driver::Driver; pub use self::driver::DriverInfo; +pub use self::driver::Registration; pub(crate) mod private { pub trait Sealed {}