From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: rust: types: Add Ownable/Owned types
Date: Mon, 23 Feb 2026 06:08:50 +1000
Message-ID: <review-patch1-20260220-unique-ref-v15-1-893ed86b06cc@kernel.org>
In-Reply-To: <20260220-unique-ref-v15-1-893ed86b06cc@kernel.org>
References: <20260220-unique-ref-v15-0-893ed86b06cc@kernel.org> <20260220-unique-ref-v15-1-893ed86b06cc@kernel.org>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

This patch introduces the core `Ownable` trait and `Owned<T>` type in a new `rust/kernel/owned.rs` file, and re-exports them from `types.rs`.

The design is clean. A few observations:

> +///         let result = NonNull::new(KBox::into_raw(result))
> +///             .expect("Raw pointer to newly allocation KBox is null, this should never happen.");

As Alice noted on the list, `KBox::into_raw` cannot return null (it returns `*mut T` from a valid allocation). A `KBox::into_raw_nonnull()` would be cleaner. This is fixed in patch 5 where the example switches to `.ok_or(ENOMEM)?`, but `ok_or(ENOMEM)` is also somewhat misleading since the null case is unreachable.

> +///    let foo = Foo::new().expect("Failed to allocate a Foo. This shouldn't happen");

Also fixed in patch 5 to use `?`.

> +/// # Invariants
> +///
> +/// - The [`Owned<T>`] has exclusive access to the instance of `T`.
> +/// - The instance of `T` will stay alive at least as long as the [`Owned<T>`] is alive.

Alice's suggestion to make the invariants more concrete (permission to call `release` once, permission for mutable access until `release`, value is pinned) would strengthen the safety reasoning throughout the series. In particular, the pinning guarantee is currently argued through "what APIs we expose" rather than as a type invariant, which makes each `Pin::new_unchecked` safety argument fragile.

> +impl<T: Ownable + Unpin> DerefMut for Owned<T> {
> +    fn deref_mut(&mut self) -> &mut Self::Target {
> +        // SAFETY: The type invariants guarantee that the object is valid, and that we can safely
> +        // return a mutable reference to it.
> +        unsafe { self.ptr.as_mut() }

As Alice noted, if "pinned" were a type invariant, this safety comment would naturally need to explain why unpinning is okay (because `T: Unpin`). Without the invariant, the safety reasoning is incomplete.

No bugs found in this patch; the issues are documentation quality.

---
Generated by Claude Code Patch Reviewer