Re: [PATCH v4 03/11] rust: xarray: add `XArrayState`

[Andreas Hindborg]

Tamir Duberstein <tamird@xxxxxxxxxx> writes:

> On Thu, 04 Jun 2026 21:58:09 +0200, Andreas Hindborg <a.hindborg@xxxxxxxxxx> wrote:
>> Add `XArrayState` as internal state for XArray iteration and entry
>> operations. This struct wraps the C `xa_state` structure and holds a
>> reference to a `Guard` to ensure exclusive access to the XArray for the
>> lifetime of the state object.
>> 
>> The `XAS_RESTART` constant is also exposed through the bindings helper
>> to properly initialize the `xa_node` field.
>> 
>> The struct and its constructor are marked with `#[expect(dead_code)]` as
>> there are no users yet. We will remove this annotation in a later patch.
>
> The review of this patch in v3 requested the next patch be absorbed into
> it.

I must have missed that.

>
>>
>>
>> diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs
>> index d54942aeb201..6d0d4905004a 100644
>> --- a/rust/kernel/xarray.rs
>> +++ b/rust/kernel/xarray.rs
>> @@ -299,6 +302,67 @@ pub fn store(
>> [ ... skip 17 lines ... ]
>> +impl<'a, T: ForeignOwnable> GuardRef for &mut Guard<'a, T> {
>> +    type Value = T;
>> +    fn xa_ptr(&self) -> *mut bindings::xarray {
>> +        self.xa.xa.get()
>> +    }
>> +}
>
> I'm having a hard time understanding the need for this trait, and it is
> not described in the commit message at all. How is this different than
> using Deref? `&mut T` has a blanket `Deref<Target = T>` impl.

I needed `XArrayState` to be generic over `&Guard<'a, T>` and `&mut
Guard<'a, T>` with ability to obtain the raw xarray pointer through the
guard, and ability to name `T`. I did not consider just using `Deref`,
but we can do that. The bounds on the impl block become a bit more
complicated, but I guess that is fine. Here is a diff (against head of series):

diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs
index cbb16368c2ca..f5a499c23778 100644
--- a/rust/kernel/xarray.rs
+++ b/rust/kernel/xarray.rs
@@ -551,27 +551,6 @@ pub fn insert_entry<'b>(
     }
 }
 
-/// A reference to a [`Guard`], either shared or mutable, that exposes the
-/// underlying xarray pointer and the value type stored in the array.
-pub(crate) trait GuardRef {
-    type Value: ForeignOwnable;
-    fn xa_ptr(&self) -> *mut bindings::xarray;
-}
-
-impl<'a, T: ForeignOwnable> GuardRef for &Guard<'a, T> {
-    type Value = T;
-    fn xa_ptr(&self) -> *mut bindings::xarray {
-        self.xa.xa.get()
-    }
-}
-
-impl<'a, T: ForeignOwnable> GuardRef for &mut Guard<'a, T> {
-    type Value = T;
-    fn xa_ptr(&self) -> *mut bindings::xarray {
-        self.xa.xa.get()
-    }
-}
-
 /// Internal state for XArray iteration and entry operations.
 ///
 /// `R` is the borrow held on the guard: either `&Guard` for read-only callers
@@ -582,12 +561,12 @@ fn xa_ptr(&self) -> *mut bindings::xarray {
 ///
 /// - `state` is always a valid `bindings::xa_state`.
 /// - `state.xa` aliases the xarray reachable through `guard`.
-pub(crate) struct XArrayState<R: GuardRef> {
+pub(crate) struct XArrayState<R> {
     guard: R,
     state: bindings::xa_state,
 }
 
-impl<R: GuardRef> Drop for XArrayState<R> {
+impl<R> Drop for XArrayState<R> {
     fn drop(&mut self) {
         free_xa_alloc(&mut self.state);
     }
@@ -611,9 +590,13 @@ fn free_xa_alloc(state: &mut bindings::xa_state) {
     }
 }
 
-impl<R: GuardRef> XArrayState<R> {
+impl<'a, R, T> XArrayState<R>
+where
+    T: ForeignOwnable + 'a,
+    R: core::ops::Deref<Target = Guard<'a, T>>
+{
     fn new(guard: R, index: usize) -> Self {
-        let xa_ptr = guard.xa_ptr();
+        let xa_ptr = guard.xa.xa.get();
         // INVARIANT: `state` is initialized to a valid `xa_state` whose `xa` field aliases the
         // xarray reachable through `guard`.
         Self {
@@ -656,10 +639,10 @@ fn status(&self) -> Result {
 
     fn insert(
         &mut self,
-        value: R::Value,
+        value: T,
         mut preload: Option<&mut XArraySheaf<'_>>,
-    ) -> Result<*mut c_void, StoreError<R::Value>> {
-        let new = R::Value::into_foreign(value).cast();
+    ) -> Result<*mut c_void, StoreError<T>> {
+        let new = T::into_foreign(value).cast();
 
         loop {
             // SAFETY: `self.state` is a valid `xa_state` by the type invariant. By the same
@@ -686,7 +669,7 @@ fn insert(
         .map_err(|error| {
             // SAFETY: `new` came from `R::Value::into_foreign` and `xas_store` does not take
             // ownership of the value on error.
-            let value = unsafe { R::Value::from_foreign(new) };
+            let value = unsafe { T::from_foreign(new) };
             StoreError { value, error }
         })
     }
---


Best regards,
Andreas Hindborg