[PATCH 1/2] Partially revert "rust: drm: gem: Implement AlwaysRefCounted for all gem objects automatically"

Lyude Paul posted 2 patches 1 day, 3 hours ago
[PATCH 1/2] Partially revert "rust: drm: gem: Implement AlwaysRefCounted for all gem objects automatically"
Posted by Lyude Paul 1 day, 3 hours ago
I made a very silly mistake with this commit that managed to slip by
because I forgot to mzke sure rvkms was rebased before testing my work last
- we can't do blanket implementations like this due to rust's orphan rule.

The code -does- build just fine right now, but it doesn't with the ongoing
bindings for gem shmem. So, just revert this and we'll introduce a macro
for implementing AlwaysRefCounted individually for each type of gem
implementation.

Note that we leave the IntoGEMObject since it is true that all gem objects
are refcounted, so any implementations that are added should be
implementing AlwaysRefCounted anyhow.

This reverts commit 38cb08c3fcd3f3b1d0225dcec8ae50fab5751549.

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 rust/kernel/drm/gem/mod.rs | 36 ++++++++++++++++--------------------
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs
index fd872de3b6695..af92f2d46d8d8 100644
--- a/rust/kernel/drm/gem/mod.rs
+++ b/rust/kernel/drm/gem/mod.rs
@@ -54,26 +54,6 @@ pub trait IntoGEMObject: Sized + super::private::Sealed + AlwaysRefCounted {
     unsafe fn from_raw<'a>(self_ptr: *mut bindings::drm_gem_object) -> &'a Self;
 }
 
-// SAFETY: All gem objects are refcounted.
-unsafe impl<T: IntoGEMObject> AlwaysRefCounted for T {
-    fn inc_ref(&self) {
-        // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
-        unsafe { bindings::drm_gem_object_get(self.as_raw()) };
-    }
-
-    unsafe fn dec_ref(obj: NonNull<Self>) {
-        // SAFETY: We either hold the only refcount on `obj`, or one of many - meaning that no one
-        // else could possibly hold a mutable reference to `obj` and thus this immutable reference
-        // is safe.
-        let obj = unsafe { obj.as_ref() }.as_raw();
-
-        // SAFETY:
-        // - The safety requirements guarantee that the refcount is non-zero.
-        // - We hold no references to `obj` now, making it safe for us to potentially deallocate it.
-        unsafe { bindings::drm_gem_object_put(obj) };
-    }
-}
-
 extern "C" fn open_callback<T: DriverObject>(
     raw_obj: *mut bindings::drm_gem_object,
     raw_file: *mut bindings::drm_file,
@@ -272,6 +252,22 @@ extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {
     }
 }
 
+// SAFETY: Instances of `Object<T>` are always reference-counted.
+unsafe impl<T: DriverObject> crate::types::AlwaysRefCounted for Object<T> {
+    fn inc_ref(&self) {
+        // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
+        unsafe { bindings::drm_gem_object_get(self.as_raw()) };
+    }
+
+    unsafe fn dec_ref(obj: NonNull<Self>) {
+        // SAFETY: `obj` is a valid pointer to an `Object<T>`.
+        let obj = unsafe { obj.as_ref() };
+
+        // SAFETY: The safety requirements guarantee that the refcount is non-zero.
+        unsafe { bindings::drm_gem_object_put(obj.as_raw()) }
+    }
+}
+
 impl<T: DriverObject> super::private::Sealed for Object<T> {}
 
 impl<T: DriverObject> Deref for Object<T> {
-- 
2.51.0
Re: [PATCH 1/2] Partially revert "rust: drm: gem: Implement AlwaysRefCounted for all gem objects automatically"
Posted by Boqun Feng 22 hours ago
Hi Lyude,

On Mon, Sep 08, 2025 at 06:04:44PM -0400, Lyude Paul wrote:
> I made a very silly mistake with this commit that managed to slip by
> because I forgot to mzke sure rvkms was rebased before testing my work last
> - we can't do blanket implementations like this due to rust's orphan rule.
> 

In general, I would avoid using "I did something" to describe the issue
of a previous commit in the commit log of a patch, it's less objective
IMO. Could you reword this a bit? Maybe more focus on why a blanket
implementation is problematic.

Thanks!

Regards,
Boqun

> The code -does- build just fine right now, but it doesn't with the ongoing
> bindings for gem shmem. So, just revert this and we'll introduce a macro
> for implementing AlwaysRefCounted individually for each type of gem
> implementation.
> 
> Note that we leave the IntoGEMObject since it is true that all gem objects
> are refcounted, so any implementations that are added should be
> implementing AlwaysRefCounted anyhow.
> 
> This reverts commit 38cb08c3fcd3f3b1d0225dcec8ae50fab5751549.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
[...]