`{into,from}_foreign` before `borrow` is slightly more logical.

Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 rust/kernel/sync/arc.rs | 18 +++++++++---------
 rust/kernel/types.rs    | 20 ++++++++++----------
 2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index 26b7272424aafdd4847d9642456cab837797ac33..4552913c75f747d646bf408c1e8a1a883afb4b6a 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -339,29 +339,29 @@ fn into_foreign(self) -> *mut core::ffi::c_void {
         ManuallyDrop::new(self).ptr.as_ptr().cast()
     }
 
-    unsafe fn borrow<'a>(ptr: *mut core::ffi::c_void) -> ArcBorrow<'a, T> {
+    unsafe fn from_foreign(ptr: *mut core::ffi::c_void) -> Self {
         let ptr = ptr.cast();
 
         // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
         // call to `Self::into_foreign`.
         let inner = unsafe { NonNull::new_unchecked(ptr) };
 
-        // SAFETY: The safety requirements of `from_foreign` ensure that the object remains alive
-        // for the lifetime of the returned value.
-        unsafe { ArcBorrow::new(inner) }
+        // SAFETY: By the safety requirement of this function, we know that `ptr` came from
+        // a previous call to `Arc::into_foreign`, which guarantees that `ptr` is valid and
+        // holds a reference count increment that is transferrable to us.
+        unsafe { Self::from_inner(inner) }
     }
 
-    unsafe fn from_foreign(ptr: *mut core::ffi::c_void) -> Self {
+    unsafe fn borrow<'a>(ptr: *mut core::ffi::c_void) -> ArcBorrow<'a, T> {
         let ptr = ptr.cast();
 
         // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
         // call to `Self::into_foreign`.
         let inner = unsafe { NonNull::new_unchecked(ptr) };
 
-        // SAFETY: By the safety requirement of this function, we know that `ptr` came from
-        // a previous call to `Arc::into_foreign`, which guarantees that `ptr` is valid and
-        // holds a reference count increment that is transferrable to us.
-        unsafe { Self::from_inner(inner) }
+        // SAFETY: The safety requirements of `from_foreign` ensure that the object remains alive
+        // for the lifetime of the returned value.
+        unsafe { ArcBorrow::new(inner) }
     }
 }
 
diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 04358375794dc5ba7bfebbe3cfc5885cff531f15..b8a7b2ee96a17081ad31e1bb73cb0513bcd05ef4 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -31,14 +31,6 @@ pub trait ForeignOwnable: Sized {
     /// [`ForeignOwnable::try_from_foreign`] can result in undefined behavior.
     fn into_foreign(self) -> *mut core::ffi::c_void;
 
-    /// Borrows a foreign-owned object.
-    ///
-    /// # Safety
-    ///
-    /// `ptr` must have been returned by a previous call to [`ForeignOwnable::into_foreign`] for
-    /// which a previous matching [`ForeignOwnable::from_foreign`] hasn't been called yet.
-    unsafe fn borrow<'a>(ptr: *mut core::ffi::c_void) -> Self::Borrowed<'a>;
-
     /// Converts a foreign-owned object back to a Rust-owned one.
     ///
     /// # Safety
@@ -67,6 +59,14 @@ unsafe fn try_from_foreign(ptr: *mut core::ffi::c_void) -> Option<Self> {
             unsafe { Some(Self::from_foreign(ptr)) }
         }
     }
+
+    /// Borrows a foreign-owned object.
+    ///
+    /// # Safety
+    ///
+    /// `ptr` must have been returned by a previous call to [`ForeignOwnable::into_foreign`] for
+    /// which a previous matching [`ForeignOwnable::from_foreign`] hasn't been called yet.
+    unsafe fn borrow<'a>(ptr: *mut core::ffi::c_void) -> Self::Borrowed<'a>;
 }
 
 impl ForeignOwnable for () {
@@ -76,9 +76,9 @@ fn into_foreign(self) -> *mut core::ffi::c_void {
         core::ptr::NonNull::dangling().as_ptr()
     }
 
-    unsafe fn borrow<'a>(_: *mut core::ffi::c_void) -> Self::Borrowed<'a> {}
-
     unsafe fn from_foreign(_: *mut core::ffi::c_void) -> Self {}
+
+    unsafe fn borrow<'a>(_: *mut core::ffi::c_void) -> Self::Borrowed<'a> {}
 }
 
 /// Runs a cleanup function/closure when dropped.

-- 
2.47.0

"Tamir Duberstein" <tamird@gmail.com> writes:

> `{into,from}_foreign` before `borrow` is slightly more logical.
>
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>

I don't think this change is required, I would drop this patch.

Best regards,
Andreas

On Thu, Oct 31, 2024 at 4:50 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> "Tamir Duberstein" <tamird@gmail.com> writes:
>
> > `{into,from}_foreign` before `borrow` is slightly more logical.
> >
> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
>
> I don't think this change is required, I would drop this patch.
>
> Best regards,
> Andreas
>

This change was part of the original patch. Do you prefer the code
movement in the same commit?

On Thu, Oct 31, 2024 at 1:23 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> This change was part of the original patch. Do you prefer the code
> movement in the same commit?

If we do it, please keep it separate, that is a good idea.

However, I think Andreas means to avoid the movement at all,
regardless of which commit is used to do it.

Cheers,
Miguel

"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes:

> On Thu, Oct 31, 2024 at 1:23 PM Tamir Duberstein <tamird@gmail.com> wrote:
>>
>> This change was part of the original patch. Do you prefer the code
>> movement in the same commit?
>
> If we do it, please keep it separate, that is a good idea.
>
> However, I think Andreas means to avoid the movement at all,
> regardless of which commit is used to do it.

Exactly.


Best regards,
Andreas Hindborg

On Thu, Oct 31, 2024 at 8:40 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Thu, Oct 31, 2024 at 1:23 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > This change was part of the original patch. Do you prefer the code
> > movement in the same commit?
>
> If we do it, please keep it separate, that is a good idea.
>
> However, I think Andreas means to avoid the movement at all,
> regardless of which commit is used to do it.

Understood. I'll update the commit message to explain that this also
avoids inconsistency with the implementation in kbox.rs, which is
already in this order.

Andreas, please let me know if this is acceptable.

Thanks.
Tamir

"Tamir Duberstein" <tamird@gmail.com> writes:

> On Thu, Oct 31, 2024 at 8:40 AM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
>>
>> On Thu, Oct 31, 2024 at 1:23 PM Tamir Duberstein <tamird@gmail.com> wrote:
>> >
>> > This change was part of the original patch. Do you prefer the code
>> > movement in the same commit?
>>
>> If we do it, please keep it separate, that is a good idea.
>>
>> However, I think Andreas means to avoid the movement at all,
>> regardless of which commit is used to do it.
>
> Understood. I'll update the commit message to explain that this also
> avoids inconsistency with the implementation in kbox.rs, which is
> already in this order.
>
> Andreas, please let me know if this is acceptable.

I would prefer not to move the code, but I am not going to make more
noise about it that this :) I don't care what order these items are, and
so to me it is just noise in the history.


Best regards,
Andreas Hindborg