From 94febfb5bcfb6ccf02283cc07bf58927c119afca Mon Sep 17 00:00:00 2001 From: Beata Michalska Date: Thu, 26 Jun 2025 18:23:13 +0200 Subject: rust: drm: Drop the use of Opaque for ioctl arguments With the Opaque, the expectations are that Rust should not make any assumptions on the layout or invariants of the wrapped C types. That runs rather counter to ioctl arguments, which must adhere to certain data-layout constraints. By using Opaque, ioctl handlers are forced to use unsafe code where none is actually needed. This adds needless complexity and maintenance overhead, brining no safety benefits. Drop the use of Opaque for ioctl arguments as that is not the best fit here. Signed-off-by: Beata Michalska Reviewed-by: Boqun Feng Reviewed-by: Daniel Almeida Reviewed-by: Alice Ryhl Link: https://lore.kernel.org/r/20250626162313.2755584-1-beata.michalska@arm.com Signed-off-by: Danilo Krummrich --- rust/kernel/drm/ioctl.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) (limited to 'rust/kernel/drm') diff --git a/rust/kernel/drm/ioctl.rs b/rust/kernel/drm/ioctl.rs index fdec01c37168..af1bb29cf06d 100644 --- a/rust/kernel/drm/ioctl.rs +++ b/rust/kernel/drm/ioctl.rs @@ -83,7 +83,7 @@ pub mod internal { /// /// ```ignore /// fn foo(device: &kernel::drm::Device, -/// data: &Opaque, +/// data: &mut uapi::argument_type, /// file: &kernel::drm::File, /// ) -> Result /// ``` @@ -138,9 +138,12 @@ macro_rules! declare_drm_ioctls { // SAFETY: The ioctl argument has size `_IOC_SIZE(cmd)`, which we // asserted above matches the size of this type, and all bit patterns of // UAPI structs must be valid. - let data = unsafe { - &*(raw_data as *const $crate::types::Opaque<$crate::uapi::$struct>) - }; + // The `ioctl` argument is exclusively owned by the handler + // and guaranteed by the C implementation (`drm_ioctl()`) to remain + // valid for the entire lifetime of the reference taken here. + // There is no concurrent access or aliasing; no other references + // to this object exist during this call. + let data = unsafe { &mut *(raw_data.cast::<$crate::uapi::$struct>()) }; // SAFETY: This is just the DRM file structure let file = unsafe { $crate::drm::File::from_raw(raw_file) }; -- cgit v1.2.3 From f1f2a22b8683d7ac38821d4508d4549a2f0c0a0a Mon Sep 17 00:00:00 2001 From: Shankari Anand Date: Fri, 15 Aug 2025 21:47:06 +0530 Subject: rust: drm: update ARef and AlwaysRefCounted imports from sync::aref Update call sites in drm to import `ARef` and `AlwaysRefCounted` from `sync::aref` instead of `types`. This aligns with the ongoing effort to move `ARef` and `AlwaysRefCounted` to sync. Suggested-by: Benno Lossin Link: https://github.com/Rust-for-Linux/linux/issues/1173 Signed-off-by: Shankari Anand Reviewed-by: Benno Lossin Reviewed-by: Elle Rhumsaa Link: https://lore.kernel.org/r/20250815161706.1324860-1-shankari.ak0208@gmail.com Signed-off-by: Danilo Krummrich --- rust/kernel/drm/device.rs | 3 ++- rust/kernel/drm/driver.rs | 2 +- rust/kernel/drm/gem/mod.rs | 3 ++- 3 files changed, 5 insertions(+), 3 deletions(-) (limited to 'rust/kernel/drm') diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs index 3bb7c83966cf..4a62f9fd88b7 100644 --- a/rust/kernel/drm/device.rs +++ b/rust/kernel/drm/device.rs @@ -10,7 +10,8 @@ use crate::{ error::from_err_ptr, error::Result, prelude::*, - types::{ARef, AlwaysRefCounted, Opaque}, + sync::aref::{ARef, AlwaysRefCounted}, + types::Opaque, }; use core::{mem, ops::Deref, ptr, ptr::NonNull}; diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs index fe7e8d06961a..8fefae41bcc6 100644 --- a/rust/kernel/drm/driver.rs +++ b/rust/kernel/drm/driver.rs @@ -8,7 +8,7 @@ use crate::{ bindings, device, devres, drm, error::{to_result, Result}, prelude::*, - types::ARef, + sync::aref::ARef, }; use macros::vtable; diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs index b71821cfb5ea..a822aedee949 100644 --- a/rust/kernel/drm/gem/mod.rs +++ b/rust/kernel/drm/gem/mod.rs @@ -10,7 +10,8 @@ use crate::{ drm::driver::{AllocImpl, AllocOps}, error::{to_result, Result}, prelude::*, - types::{ARef, AlwaysRefCounted, Opaque}, + sync::aref::{ARef, AlwaysRefCounted}, + types::Opaque, }; use core::{mem, ops::Deref, ptr::NonNull}; -- cgit v1.2.3 From 6ea42e9146f7ab8e59ffea8aa3300ad6710399dd Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Mon, 8 Sep 2025 14:46:36 -0400 Subject: rust: drm: gem: Simplify use of generics Now that my rust skills have been honed, I noticed that there's a lot of generics in our gem bindings that don't actually need to be here. Currently the hierarchy of traits in our gem bindings looks like this: * Drivers implement: * BaseDriverObject (has the callbacks) * DriverObject (has the drm::Driver type) * Crate implements: * IntoGEMObject for Object where T: DriverObject Handles conversion to/from raw object pointers * BaseObject for T where T: IntoGEMObject Provides methods common to all gem interfaces Also of note, this leaves us with two different drm::Driver associated types: * DriverObject::Driver * IntoGEMObject::Driver I'm not entirely sure of the original intent here unfortunately (if anyone is, please let me know!), but my guess is that the idea would be that some objects can implement IntoGEMObject using a different ::Driver than DriverObject - presumably to enable the usage of gem objects from different drivers. A reasonable usecase of course. However - if I'm not mistaken, I don't think that this is actually how things would go in practice. Driver implementations are of course implemented by their associated drivers, and generally drivers are not linked to each-other when building the kernel. Which is to say that even in a situation where we would theoretically deal with gem objects from another driver, we still wouldn't have access to its drm::driver::Driver implementation. It's more likely we would simply want a variant of gem objects in such a situation that have no association with a drm::driver::Driver type. Taking that into consideration, we can assume the following: * Anything that implements BaseDriverObject will implement DriverObject In other words, all BaseDriverObjects indirectly have an associated ::Driver type - so the two traits can be combined into one with no generics. * Not everything that implements IntoGEMObject will have an associated ::Driver, and that's OK. And with this, we now can do quite a bit of cleanup with the use of generics here. As such, this commit: * Removes the generics on BaseDriverObject * Moves DriverObject::Driver into BaseDriverObject * Removes DriverObject * Removes IntoGEMObject::Driver * Add AllocImpl::Driver, which we can use as a binding to figure out the correct File type for BaseObject Leaving us with a simpler trait hierarchy that now looks like this: * Drivers implement: BaseDriverObject * Crate implements: * IntoGEMObject for Object where T: DriverObject * BaseObject for T where T: IntoGEMObject Which makes the code a lot easier to understand and build on :). Signed-off-by: Lyude Paul Reviewed-by: Daniel Almeida Acked-by: Danilo Krummrich Reviewed-by: Alice Ryhl Link: https://lore.kernel.org/r/20250908185239.135849-2-lyude@redhat.com Signed-off-by: Alice Ryhl --- drivers/gpu/drm/nova/gem.rs | 8 ++--- rust/kernel/drm/driver.rs | 3 ++ rust/kernel/drm/gem/mod.rs | 77 ++++++++++++++++++++------------------------- 3 files changed, 40 insertions(+), 48 deletions(-) (limited to 'rust/kernel/drm') diff --git a/drivers/gpu/drm/nova/gem.rs b/drivers/gpu/drm/nova/gem.rs index cd82773dab92..2760ba4f3450 100644 --- a/drivers/gpu/drm/nova/gem.rs +++ b/drivers/gpu/drm/nova/gem.rs @@ -16,16 +16,14 @@ use crate::{ #[pin_data] pub(crate) struct NovaObject {} -impl gem::BaseDriverObject> for NovaObject { +impl gem::DriverObject for NovaObject { + type Driver = NovaDriver; + fn new(_dev: &NovaDevice, _size: usize) -> impl PinInit { try_pin_init!(NovaObject {}) } } -impl gem::DriverObject for NovaObject { - type Driver = NovaDriver; -} - impl NovaObject { /// Create a new DRM GEM object. pub(crate) fn new(dev: &NovaDevice, size: usize) -> Result>> { diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs index fe7e8d06961a..dae0f4d1bbe3 100644 --- a/rust/kernel/drm/driver.rs +++ b/rust/kernel/drm/driver.rs @@ -86,6 +86,9 @@ pub struct AllocOps { /// Trait for memory manager implementations. Implemented internally. pub trait AllocImpl: super::private::Sealed + drm::gem::IntoGEMObject { + /// The [`Driver`] implementation for this [`AllocImpl`]. + type Driver: drm::Driver; + /// The C callback operations for this memory manager. const ALLOC_OPS: AllocOps; } diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs index b71821cfb5ea..f5b19865d745 100644 --- a/rust/kernel/drm/gem/mod.rs +++ b/rust/kernel/drm/gem/mod.rs @@ -15,31 +15,31 @@ use crate::{ use core::{mem, ops::Deref, ptr::NonNull}; /// GEM object functions, which must be implemented by drivers. -pub trait BaseDriverObject: Sync + Send + Sized { +pub trait DriverObject: Sync + Send + Sized { + /// Parent `Driver` for this object. + type Driver: drm::Driver; + /// Create a new driver data object for a GEM object of a given size. - fn new(dev: &drm::Device, size: usize) -> impl PinInit; + fn new(dev: &drm::Device, size: usize) -> impl PinInit; /// Open a new handle to an existing object, associated with a File. fn open( - _obj: &<::Driver as drm::Driver>::Object, - _file: &drm::File<<::Driver as drm::Driver>::File>, + _obj: &::Object, + _file: &drm::File<::File>, ) -> Result { Ok(()) } /// Close a handle to an existing object, associated with a File. fn close( - _obj: &<::Driver as drm::Driver>::Object, - _file: &drm::File<<::Driver as drm::Driver>::File>, + _obj: &::Object, + _file: &drm::File<::File>, ) { } } /// Trait that represents a GEM object subtype pub trait IntoGEMObject: Sized + super::private::Sealed + AlwaysRefCounted { - /// Owning driver for this type - type Driver: drm::Driver; - /// Returns a reference to the raw `drm_gem_object` structure, which must be valid as long as /// this owning object is valid. fn as_raw(&self) -> *mut bindings::drm_gem_object; @@ -74,25 +74,15 @@ unsafe impl AlwaysRefCounted for T { } } -/// Trait which must be implemented by drivers using base GEM objects. -pub trait DriverObject: BaseDriverObject> { - /// Parent `Driver` for this object. - type Driver: drm::Driver; -} - -extern "C" fn open_callback, U: BaseObject>( +extern "C" fn open_callback( raw_obj: *mut bindings::drm_gem_object, raw_file: *mut bindings::drm_file, ) -> core::ffi::c_int { // SAFETY: `open_callback` is only ever called with a valid pointer to a `struct drm_file`. - let file = unsafe { - drm::File::<<::Driver as drm::Driver>::File>::from_raw(raw_file) - }; - // SAFETY: `open_callback` is specified in the AllocOps structure for `Object`, ensuring that - // `raw_obj` is indeed contained within a `Object`. - let obj = unsafe { - <<::Driver as drm::Driver>::Object as IntoGEMObject>::from_raw(raw_obj) - }; + let file = unsafe { drm::File::<::File>::from_raw(raw_file) }; + // SAFETY: `open_callback` is specified in the AllocOps structure for `DriverObject`, + // ensuring that `raw_obj` is contained within a `DriverObject` + let obj = unsafe { <::Object as IntoGEMObject>::from_raw(raw_obj) }; match T::open(obj, file) { Err(e) => e.to_errno(), @@ -100,26 +90,21 @@ extern "C" fn open_callback, U: BaseObject>( } } -extern "C" fn close_callback, U: BaseObject>( +extern "C" fn close_callback( raw_obj: *mut bindings::drm_gem_object, raw_file: *mut bindings::drm_file, ) { // SAFETY: `open_callback` is only ever called with a valid pointer to a `struct drm_file`. - let file = unsafe { - drm::File::<<::Driver as drm::Driver>::File>::from_raw(raw_file) - }; + let file = unsafe { drm::File::<::File>::from_raw(raw_file) }; + // SAFETY: `close_callback` is specified in the AllocOps structure for `Object`, ensuring // that `raw_obj` is indeed contained within a `Object`. - let obj = unsafe { - <<::Driver as drm::Driver>::Object as IntoGEMObject>::from_raw(raw_obj) - }; + let obj = unsafe { <::Object as IntoGEMObject>::from_raw(raw_obj) }; T::close(obj, file); } impl IntoGEMObject for Object { - type Driver = T::Driver; - fn as_raw(&self) -> *mut bindings::drm_gem_object { self.obj.get() } @@ -141,10 +126,12 @@ pub trait BaseObject: IntoGEMObject { /// Creates a new handle for the object associated with a given `File` /// (or returns an existing one). - fn create_handle( - &self, - file: &drm::File<<::Driver as drm::Driver>::File>, - ) -> Result { + fn create_handle(&self, file: &drm::File) -> Result + where + Self: AllocImpl, + D: drm::Driver, + F: drm::file::DriverFile, + { let mut handle: u32 = 0; // SAFETY: The arguments are all valid per the type invariants. to_result(unsafe { @@ -154,10 +141,12 @@ pub trait BaseObject: IntoGEMObject { } /// Looks up an object by its handle for a given `File`. - fn lookup_handle( - file: &drm::File<<::Driver as drm::Driver>::File>, - handle: u32, - ) -> Result> { + fn lookup_handle(file: &drm::File, handle: u32) -> Result> + where + Self: AllocImpl, + D: drm::Driver, + F: drm::file::DriverFile, + { // SAFETY: The arguments are all valid per the type invariants. let ptr = unsafe { bindings::drm_gem_object_lookup(file.as_raw().cast(), handle) }; if ptr.is_null() { @@ -212,8 +201,8 @@ impl Object { const OBJECT_FUNCS: bindings::drm_gem_object_funcs = bindings::drm_gem_object_funcs { free: Some(Self::free_callback), - open: Some(open_callback::>), - close: Some(close_callback::>), + open: Some(open_callback::), + close: Some(close_callback::), print_info: None, export: None, pin: None, @@ -296,6 +285,8 @@ impl Deref for Object { } impl AllocImpl for Object { + type Driver = T::Driver; + const ALLOC_OPS: AllocOps = AllocOps { gem_create_object: None, prime_handle_to_fd: None, -- cgit v1.2.3 From 1ed10db60f47306c1fd395f87bf6d443926a9488 Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Mon, 8 Sep 2025 14:46:37 -0400 Subject: rust: drm: gem: Add DriverFile type alias MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Just to reduce the clutter with the File<…> types in gem.rs. Signed-off-by: Lyude Paul Reviewed-by: Daniel Almeida Acked-by: Danilo Krummrich Reviewed-by: Alice Ryhl Link: https://lore.kernel.org/r/20250908185239.135849-3-lyude@redhat.com Signed-off-by: Alice Ryhl --- rust/kernel/drm/gem/mod.rs | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) (limited to 'rust/kernel/drm') diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs index f5b19865d745..ead723980b27 100644 --- a/rust/kernel/drm/gem/mod.rs +++ b/rust/kernel/drm/gem/mod.rs @@ -14,6 +14,13 @@ use crate::{ }; use core::{mem, ops::Deref, ptr::NonNull}; +/// A type alias for retrieving a [`Driver`]s [`DriverFile`] implementation from its +/// [`DriverObject`] implementation. +/// +/// [`Driver`]: drm::Driver +/// [`DriverFile`]: drm::file::DriverFile +pub type DriverFile = drm::File<<::Driver as drm::Driver>::File>; + /// GEM object functions, which must be implemented by drivers. pub trait DriverObject: Sync + Send + Sized { /// Parent `Driver` for this object. @@ -23,19 +30,12 @@ pub trait DriverObject: Sync + Send + Sized { fn new(dev: &drm::Device, size: usize) -> impl PinInit; /// Open a new handle to an existing object, associated with a File. - fn open( - _obj: &::Object, - _file: &drm::File<::File>, - ) -> Result { + fn open(_obj: &::Object, _file: &DriverFile) -> Result { Ok(()) } /// Close a handle to an existing object, associated with a File. - fn close( - _obj: &::Object, - _file: &drm::File<::File>, - ) { - } + fn close(_obj: &::Object, _file: &DriverFile) {} } /// Trait that represents a GEM object subtype @@ -79,7 +79,8 @@ extern "C" fn open_callback( raw_file: *mut bindings::drm_file, ) -> core::ffi::c_int { // SAFETY: `open_callback` is only ever called with a valid pointer to a `struct drm_file`. - let file = unsafe { drm::File::<::File>::from_raw(raw_file) }; + let file = unsafe { DriverFile::::from_raw(raw_file) }; + // SAFETY: `open_callback` is specified in the AllocOps structure for `DriverObject`, // ensuring that `raw_obj` is contained within a `DriverObject` let obj = unsafe { <::Object as IntoGEMObject>::from_raw(raw_obj) }; @@ -95,7 +96,7 @@ extern "C" fn close_callback( raw_file: *mut bindings::drm_file, ) { // SAFETY: `open_callback` is only ever called with a valid pointer to a `struct drm_file`. - let file = unsafe { drm::File::<::File>::from_raw(raw_file) }; + let file = unsafe { DriverFile::::from_raw(raw_file) }; // SAFETY: `close_callback` is specified in the AllocOps structure for `Object`, ensuring // that `raw_obj` is indeed contained within a `Object`. -- cgit v1.2.3 From 6b35936f058d0cb9171c7be1424b62017b874913 Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Mon, 8 Sep 2025 14:46:38 -0400 Subject: rust: drm: gem: Drop Object::SIZE Drive-by fix, it doesn't seem like anything actually uses this constant anymore. Signed-off-by: Lyude Paul Reviewed-by: Danilo Krummrich Reviewed-by: Daniel Almeida Reviewed-by: Alice Ryhl Link: https://lore.kernel.org/r/20250908185239.135849-4-lyude@redhat.com Signed-off-by: Alice Ryhl --- rust/kernel/drm/gem/mod.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) (limited to 'rust/kernel/drm') diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs index ead723980b27..fd872de3b669 100644 --- a/rust/kernel/drm/gem/mod.rs +++ b/rust/kernel/drm/gem/mod.rs @@ -12,7 +12,7 @@ use crate::{ prelude::*, types::{ARef, AlwaysRefCounted, Opaque}, }; -use core::{mem, ops::Deref, ptr::NonNull}; +use core::{ops::Deref, ptr::NonNull}; /// A type alias for retrieving a [`Driver`]s [`DriverFile`] implementation from its /// [`DriverObject`] implementation. @@ -197,9 +197,6 @@ pub struct Object { } impl Object { - /// The size of this object's structure. - pub const SIZE: usize = mem::size_of::(); - const OBJECT_FUNCS: bindings::drm_gem_object_funcs = bindings::drm_gem_object_funcs { free: Some(Self::free_callback), open: Some(open_callback::), -- cgit v1.2.3