public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] rust: drm: gem: clean up GEM state in init failure case
@ 2026-04-23 12:36 Eliot Courtney
  2026-04-23 17:15 ` Onur Özkan
  0 siblings, 1 reply; 2+ messages in thread
From: Eliot Courtney @ 2026-04-23 12:36 UTC (permalink / raw)
  To: Danilo Krummrich, Alice Ryhl, David Airlie, Simona Vetter,
	Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
	Benno Lossin, Andreas Hindborg, Trevor Gross, Alexandre Courbot
  Cc: John Hubbard, Alistair Popple, Joel Fernandes, Timur Tabi,
	dri-devel, rust-for-linux, linux-kernel, Eliot Courtney

Currently, if `drm_gem_object_init` fails, the object is freed without
any cleanup. Perform the cleanup in that case.

Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
I looked at `drm_gem_shmem_init` for an example, and it seems like the
correct cleanup here is to do `drm_gem_private_object_fini` if
`drm_gem_object_init` fails. Other C drivers do different things, but
looking at the implementation of `drm_gem_object_init`, this looks like
the only thing we need to clean up.
---
 rust/kernel/drm/gem/mod.rs | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs
index 75acda7ba500..7b6a085ace27 100644
--- a/rust/kernel/drm/gem/mod.rs
+++ b/rust/kernel/drm/gem/mod.rs
@@ -278,7 +278,14 @@ pub fn new(dev: &drm::Device<T::Driver>, size: usize, args: T::Args) -> Result<A
         unsafe { (*obj.as_raw()).funcs = &Self::OBJECT_FUNCS };
 
         // SAFETY: The arguments are all valid per the type invariants.
-        to_result(unsafe { bindings::drm_gem_object_init(dev.as_raw(), obj.obj.get(), size) })?;
+        if let Err(err) =
+            to_result(unsafe { bindings::drm_gem_object_init(dev.as_raw(), obj.obj.get(), size) })
+        {
+            // SAFETY: `drm_gem_object_init()` initializes the private GEM object state before
+            // failing, so `drm_gem_private_object_fini()` is the matching cleanup.
+            unsafe { bindings::drm_gem_private_object_fini(obj.obj.get()) };
+            return Err(err);
+        }
 
         // SAFETY: We will never move out of `Self` as `ARef<Self>` is always treated as pinned.
         let ptr = KBox::into_raw(unsafe { Pin::into_inner_unchecked(obj) });

---
base-commit: a7a080bb4236ebe577b6776d940d1717912ff6dd
change-id: 20260423-fix-gem-1-6c68b9fa0972

Best regards,
--  
Eliot Courtney <ecourtney@nvidia.com>


^ permalink raw reply related	[flat|nested] 2+ messages in thread

* Re: [PATCH] rust: drm: gem: clean up GEM state in init failure case
  2026-04-23 12:36 [PATCH] rust: drm: gem: clean up GEM state in init failure case Eliot Courtney
@ 2026-04-23 17:15 ` Onur Özkan
  0 siblings, 0 replies; 2+ messages in thread
From: Onur Özkan @ 2026-04-23 17:15 UTC (permalink / raw)
  To: Eliot Courtney
  Cc: Danilo Krummrich, Alice Ryhl, David Airlie, Simona Vetter,
	Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
	Benno Lossin, Andreas Hindborg, Trevor Gross, Alexandre Courbot,
	John Hubbard, Alistair Popple, Joel Fernandes, Timur Tabi,
	dri-devel, rust-for-linux, linux-kernel, Onur Özkan

On Thu, 23 Apr 2026 21:36:52 +0900
Eliot Courtney <ecourtney@nvidia.com> wrote:

> Currently, if `drm_gem_object_init` fails, the object is freed without
> any cleanup. Perform the cleanup in that case.
> 
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
> I looked at `drm_gem_shmem_init` for an example, and it seems like the
> correct cleanup here is to do `drm_gem_private_object_fini` if
> `drm_gem_object_init` fails. Other C drivers do different things, but
> looking at the implementation of `drm_gem_object_init`, this looks like
> the only thing we need to clean up.
> ---
>  rust/kernel/drm/gem/mod.rs | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs
> index 75acda7ba500..7b6a085ace27 100644
> --- a/rust/kernel/drm/gem/mod.rs
> +++ b/rust/kernel/drm/gem/mod.rs
> @@ -278,7 +278,14 @@ pub fn new(dev: &drm::Device<T::Driver>, size: usize, args: T::Args) -> Result<A
>          unsafe { (*obj.as_raw()).funcs = &Self::OBJECT_FUNCS };
>  
>          // SAFETY: The arguments are all valid per the type invariants.
> -        to_result(unsafe { bindings::drm_gem_object_init(dev.as_raw(), obj.obj.get(), size) })?;
> +        if let Err(err) =
> +            to_result(unsafe { bindings::drm_gem_object_init(dev.as_raw(), obj.obj.get(), size) })
> +        {
> +            // SAFETY: `drm_gem_object_init()` initializes the private GEM object state before
> +            // failing, so `drm_gem_private_object_fini()` is the matching cleanup.
> +            unsafe { bindings::drm_gem_private_object_fini(obj.obj.get()) };
> +            return Err(err);
> +        }

I can confirm that this matches the usage on the C side. I think the API could
be improved to avoid relying on developers remembering this pattern. I don't
expect that to happen in this patch, just saying it :).

Reviewed-by: Onur Özkan <work@onurozkan.dev>

>  
>          // SAFETY: We will never move out of `Self` as `ARef<Self>` is always treated as pinned.
>          let ptr = KBox::into_raw(unsafe { Pin::into_inner_unchecked(obj) });
> 
> ---
> base-commit: a7a080bb4236ebe577b6776d940d1717912ff6dd
> change-id: 20260423-fix-gem-1-6c68b9fa0972
> 
> Best regards,
> --  
> Eliot Courtney <ecourtney@nvidia.com>
> 

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2026-04-23 17:15 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-23 12:36 [PATCH] rust: drm: gem: clean up GEM state in init failure case Eliot Courtney
2026-04-23 17:15 ` Onur Özkan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox