[PATCH v16 0/4] rust: xarray: Add a minimal abstraction for XArray

[PATCH v16 0/4] rust: xarray: Add a minimal abstraction for XArray

[Tamir Duberstein]

This is a reimagining relative to earlier versions[0] by Asahi Lina and
Maíra Canal.

It is needed to support rust-binder, though this version only provides
enough machinery to support rnull.

Link: https://lore.kernel.org/rust-for-linux/20240309235927.168915-2-mcanal@igalia.com/ [0]
---
Changes in v16:
- Extract prequel patch for `as _` casts. (Danilo Krummrich)
- Improve doc and safety comments. (Boqun Feng)
- Pull trailers for shared commit from configfs series[0]. (Andreas Hindborg, Alice Ryhl, Fiona Behrens)
- Link to configfs series: https://lore.kernel.org/rust-for-linux/20250131-configfs-v1-1-87947611401c@kernel.org/
- Link to v15: https://lore.kernel.org/r/20250206-rust-xarray-bindings-v15-0-a22b5dcacab3@gmail.com

Changes in v15:
- Rebase on v6.14-rc1.
- Add MAINTAINERS entry.
- Link to v14: https://lore.kernel.org/r/20241217-rust-xarray-bindings-v14-0-9fef3cefcb41@gmail.com

Changes in v14:
- Remove TODO made stale by Gary Guo's FFI type series.
- Link: https://lore.kernel.org/all/20240913213041.395655-5-gary@garyguo.net/
- Link to v13: https://lore.kernel.org/r/20241213-rust-xarray-bindings-v13-0-8655164e624f@gmail.com

Changes in v13:
- Replace `bool::then` with `if`. (Miguel Ojeda)
- Replace `match` with `let` + `if`. (Miguel Ojeda)
- Link to v12: https://lore.kernel.org/r/20241212-rust-xarray-bindings-v12-0-59ab9b1f4d2e@gmail.com

Changes in v12:
- Import `core::ptr::NonNull`. (Alice Ryhl)
- Introduce `StoreError` to allow `?` to be used with `Guard::store`.
  (Alice Ryhl)
- Replace `{crate,core}::ffi::c_ulong` and clarify TODO with respect to
  `usize`. (Alice Ryhl)
- Drop `T: Sync` bound on `impl Sync for XArray<T>`. (Alice Ryhl)
- Reword `Send` and `Sync` safety comments to match the style used in
  `lock.rs`. (Alice Ryhl and Andreas
  Hindborg)
- Link to v11: https://lore.kernel.org/r/20241203-rust-xarray-bindings-v11-0-58a95d137ec2@gmail.com

Changes in v11:
- Consolidate imports. (Alice Ryhl)
- Use literal `0` rather than `MIN`. (Alice Ryhl)
- Use bulleted list in SAFETY comment. (Alice Ryhl)
- Document (un)locking behavior of `Guard::store`. (Alice Ryhl)
- Document Normal API behavior WRT `XA_ZERO_ENTRY`. (Alice Ryhl)
- Rewrite `unsafe impl Sync` SAFETY comment. (Andreas Hindborg)
- Link to v10: https://lore.kernel.org/r/20241120-rust-xarray-bindings-v10-0-a25b2b0bf582@gmail.com

Changes in v10:
- Guard::get takes &self instead of &mut self. (Andreas Hindborg)
- Guard is !Send. (Boqun Feng)
- Add Inspired-by tags. (Maíra Canal and Asahi Lina)
- Rebase on linux-next, use NotThreadSafe. (Alice Ryhl)
- Link to v9: https://lore.kernel.org/r/20241118-rust-xarray-bindings-v9-0-3219cdb53685@gmail.com

---
Tamir Duberstein (4):
      rust: remove redundant `as _` casts
      rust: types: add `ForeignOwnable::PointedTo`
      rust: xarray: Add an abstraction for XArray
      MAINTAINERS: add entry for Rust XArray API

 MAINTAINERS                     |  11 ++
 rust/bindings/bindings_helper.h |   6 +
 rust/helpers/helpers.c          |   1 +
 rust/helpers/xarray.c           |  28 ++++
 rust/kernel/alloc.rs            |   5 +
 rust/kernel/alloc/kbox.rs       |  38 +++---
 rust/kernel/lib.rs              |   1 +
 rust/kernel/miscdevice.rs       |   7 +-
 rust/kernel/pci.rs              |   5 +-
 rust/kernel/platform.rs         |   5 +-
 rust/kernel/sync/arc.rs         |  21 +--
 rust/kernel/types.rs            |  46 ++++---
 rust/kernel/xarray.rs           | 276 ++++++++++++++++++++++++++++++++++++++++
 13 files changed, 405 insertions(+), 45 deletions(-)
---
base-commit: 5652819e2b9232489eaefd019675abd55a442b62
change-id: 20241020-rust-xarray-bindings-bef514142968

Best regards,
-- 
Tamir Duberstein <tamird@gmail.com>

[PATCH v16 1/4] rust: remove redundant `as _` casts

[Tamir Duberstein]

Remove redundant casts added in commit 1bd8b6b2c5d3 ("rust: pci: add
basic PCI device / driver abstractions") and commit 683a63befc73 ("rust:
platform: add basic platform device / driver abstractions")

While I'm churning this line, move the `.into_foreign()` call to its own
statement to avoid churn in the next commit which adds a `.cast()` call.

Fixes: 1bd8b6b2c5d3 ("rust: pci: add basic PCI device / driver abstractions")
Fixes: 683a63befc73 ("rust: platform: add basic platform device / driver abstractions")
Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 rust/kernel/pci.rs      | 3 ++-
 rust/kernel/platform.rs | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 4c98b5b9aa1e..6c3bc14b42ad 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -72,10 +72,11 @@ extern "C" fn probe_callback(
 
         match T::probe(&mut pdev, info) {
             Ok(data) => {
+                let data = data.into_foreign();
                 // Let the `struct pci_dev` own a reference of the driver's private data.
                 // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
                 // `struct pci_dev`.
-                unsafe { bindings::pci_set_drvdata(pdev.as_raw(), data.into_foreign() as _) };
+                unsafe { bindings::pci_set_drvdata(pdev.as_raw(), data) };
             }
             Err(err) => return Error::to_errno(err),
         }
diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index 50e6b0421813..dea104563fa9 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -63,10 +63,11 @@ extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> kernel::ff
         let info = <Self as driver::Adapter>::id_info(pdev.as_ref());
         match T::probe(&mut pdev, info) {
             Ok(data) => {
+                let data = data.into_foreign();
                 // Let the `struct platform_device` own a reference of the driver's private data.
                 // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
                 // `struct platform_device`.
-                unsafe { bindings::platform_set_drvdata(pdev.as_raw(), data.into_foreign() as _) };
+                unsafe { bindings::platform_set_drvdata(pdev.as_raw(), data) };
             }
             Err(err) => return Error::to_errno(err),
         }

-- 
2.48.1

[PATCH v16 2/4] rust: types: add `ForeignOwnable::PointedTo`

[Tamir Duberstein]

Allow implementors to specify the foreign pointer type; this exposes
information about the pointed-to type such as its alignment.

This requires the trait to be `unsafe` since it is now possible for
implementors to break soundness by returning a misaligned pointer.

Encoding the pointer type in the trait (and avoiding pointer casts)
allows the compiler to check that implementors return the correct
pointer type. This is preferable to directly encoding the alignment in
the trait using a constant as the compiler would be unable to check it.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
Reviewed-by: Fiona Behrens <me@kloenk.dev>
Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 rust/kernel/alloc/kbox.rs | 38 ++++++++++++++++++++------------------
 rust/kernel/miscdevice.rs |  7 ++++++-
 rust/kernel/pci.rs        |  2 ++
 rust/kernel/platform.rs   |  2 ++
 rust/kernel/sync/arc.rs   | 21 ++++++++++++---------
 rust/kernel/types.rs      | 46 +++++++++++++++++++++++++++++++---------------
 6 files changed, 73 insertions(+), 43 deletions(-)

diff --git a/rust/kernel/alloc/kbox.rs b/rust/kernel/alloc/kbox.rs
index cb4ebea3b074..55529832db54 100644
--- a/rust/kernel/alloc/kbox.rs
+++ b/rust/kernel/alloc/kbox.rs
@@ -349,68 +349,70 @@ fn try_init<E>(init: impl Init<T, E>, flags: Flags) -> Result<Self, E>
     }
 }
 
-impl<T: 'static, A> ForeignOwnable for Box<T, A>
+// SAFETY: The `into_foreign` function returns a pointer that is well-aligned.
+unsafe impl<T: 'static, A> ForeignOwnable for Box<T, A>
 where
     A: Allocator,
 {
+    type PointedTo = T;
     type Borrowed<'a> = &'a T;
     type BorrowedMut<'a> = &'a mut T;
 
-    fn into_foreign(self) -> *mut crate::ffi::c_void {
-        Box::into_raw(self).cast()
+    fn into_foreign(self) -> *mut Self::PointedTo {
+        Box::into_raw(self)
     }
 
-    unsafe fn from_foreign(ptr: *mut crate::ffi::c_void) -> Self {
+    unsafe fn from_foreign(ptr: *mut Self::PointedTo) -> Self {
         // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
         // call to `Self::into_foreign`.
-        unsafe { Box::from_raw(ptr.cast()) }
+        unsafe { Box::from_raw(ptr) }
     }
 
-    unsafe fn borrow<'a>(ptr: *mut crate::ffi::c_void) -> &'a T {
+    unsafe fn borrow<'a>(ptr: *mut Self::PointedTo) -> &'a T {
         // SAFETY: The safety requirements of this method ensure that the object remains alive and
         // immutable for the duration of 'a.
-        unsafe { &*ptr.cast() }
+        unsafe { &*ptr }
     }
 
-    unsafe fn borrow_mut<'a>(ptr: *mut crate::ffi::c_void) -> &'a mut T {
-        let ptr = ptr.cast();
+    unsafe fn borrow_mut<'a>(ptr: *mut Self::PointedTo) -> &'a mut T {
         // SAFETY: The safety requirements of this method ensure that the pointer is valid and that
         // nothing else will access the value for the duration of 'a.
         unsafe { &mut *ptr }
     }
 }
 
-impl<T: 'static, A> ForeignOwnable for Pin<Box<T, A>>
+// SAFETY: The `into_foreign` function returns a pointer that is well-aligned.
+unsafe impl<T: 'static, A> ForeignOwnable for Pin<Box<T, A>>
 where
     A: Allocator,
 {
+    type PointedTo = T;
     type Borrowed<'a> = Pin<&'a T>;
     type BorrowedMut<'a> = Pin<&'a mut T>;
 
-    fn into_foreign(self) -> *mut crate::ffi::c_void {
+    fn into_foreign(self) -> *mut Self::PointedTo {
         // SAFETY: We are still treating the box as pinned.
-        Box::into_raw(unsafe { Pin::into_inner_unchecked(self) }).cast()
+        Box::into_raw(unsafe { Pin::into_inner_unchecked(self) })
     }
 
-    unsafe fn from_foreign(ptr: *mut crate::ffi::c_void) -> Self {
+    unsafe fn from_foreign(ptr: *mut Self::PointedTo) -> Self {
         // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
         // call to `Self::into_foreign`.
-        unsafe { Pin::new_unchecked(Box::from_raw(ptr.cast())) }
+        unsafe { Pin::new_unchecked(Box::from_raw(ptr)) }
     }
 
-    unsafe fn borrow<'a>(ptr: *mut crate::ffi::c_void) -> Pin<&'a T> {
+    unsafe fn borrow<'a>(ptr: *mut Self::PointedTo) -> Pin<&'a T> {
         // SAFETY: The safety requirements for this function ensure that the object is still alive,
         // so it is safe to dereference the raw pointer.
         // The safety requirements of `from_foreign` also ensure that the object remains alive for
         // the lifetime of the returned value.
-        let r = unsafe { &*ptr.cast() };
+        let r = unsafe { &*ptr };
 
         // SAFETY: This pointer originates from a `Pin<Box<T>>`.
         unsafe { Pin::new_unchecked(r) }
     }
 
-    unsafe fn borrow_mut<'a>(ptr: *mut crate::ffi::c_void) -> Pin<&'a mut T> {
-        let ptr = ptr.cast();
+    unsafe fn borrow_mut<'a>(ptr: *mut Self::PointedTo) -> Pin<&'a mut T> {
         // SAFETY: The safety requirements for this function ensure that the object is still alive,
         // so it is safe to dereference the raw pointer.
         // The safety requirements of `from_foreign` also ensure that the object remains alive for
diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
index e14433b2ab9d..f1a081dd64c7 100644
--- a/rust/kernel/miscdevice.rs
+++ b/rust/kernel/miscdevice.rs
@@ -225,13 +225,15 @@ impl<T: MiscDevice> VtableHelper<T> {
         Ok(ptr) => ptr,
         Err(err) => return err.to_errno(),
     };
+    let ptr = ptr.into_foreign();
+    let ptr = ptr.cast();
 
     // This overwrites the private data with the value specified by the user, changing the type of
     // this file's private data. All future accesses to the private data is performed by other
     // fops_* methods in this file, which all correctly cast the private data to the new type.
     //
     // SAFETY: The open call of a file can access the private data.
-    unsafe { (*raw_file).private_data = ptr.into_foreign() };
+    unsafe { (*raw_file).private_data = ptr };
 
     0
 }
@@ -246,6 +248,7 @@ impl<T: MiscDevice> VtableHelper<T> {
 ) -> c_int {
     // SAFETY: The release call of a file owns the private data.
     let private = unsafe { (*file).private_data };
+    let private = private.cast();
     // SAFETY: The release call of a file owns the private data.
     let ptr = unsafe { <T::Ptr as ForeignOwnable>::from_foreign(private) };
 
@@ -267,6 +270,7 @@ impl<T: MiscDevice> VtableHelper<T> {
 ) -> c_long {
     // SAFETY: The ioctl call of a file can access the private data.
     let private = unsafe { (*file).private_data };
+    let private = private.cast();
     // SAFETY: Ioctl calls can borrow the private data of the file.
     let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
 
@@ -316,6 +320,7 @@ impl<T: MiscDevice> VtableHelper<T> {
 ) {
     // SAFETY: The release call of a file owns the private data.
     let private = unsafe { (*file).private_data };
+    let private = private.cast();
     // SAFETY: Ioctl calls can borrow the private data of the file.
     let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
     // SAFETY:
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 6c3bc14b42ad..eb25fabbff9c 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -73,6 +73,7 @@ extern "C" fn probe_callback(
         match T::probe(&mut pdev, info) {
             Ok(data) => {
                 let data = data.into_foreign();
+                let data = data.cast();
                 // Let the `struct pci_dev` own a reference of the driver's private data.
                 // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
                 // `struct pci_dev`.
@@ -88,6 +89,7 @@ extern "C" fn remove_callback(pdev: *mut bindings::pci_dev) {
         // SAFETY: The PCI bus only ever calls the remove callback with a valid pointer to a
         // `struct pci_dev`.
         let ptr = unsafe { bindings::pci_get_drvdata(pdev) };
+        let ptr = ptr.cast();
 
         // SAFETY: `remove_callback` is only ever called after a successful call to
         // `probe_callback`, hence it's guaranteed that `ptr` points to a valid and initialized
diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index dea104563fa9..53764cb7f804 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -64,6 +64,7 @@ extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> kernel::ff
         match T::probe(&mut pdev, info) {
             Ok(data) => {
                 let data = data.into_foreign();
+                let data = data.cast();
                 // Let the `struct platform_device` own a reference of the driver's private data.
                 // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
                 // `struct platform_device`.
@@ -78,6 +79,7 @@ extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> kernel::ff
     extern "C" fn remove_callback(pdev: *mut bindings::platform_device) {
         // SAFETY: `pdev` is a valid pointer to a `struct platform_device`.
         let ptr = unsafe { bindings::platform_get_drvdata(pdev) };
+        let ptr = ptr.cast();
 
         // SAFETY: `remove_callback` is only ever called after a successful call to
         // `probe_callback`, hence it's guaranteed that `ptr` points to a valid and initialized
diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index 3cefda7a4372..dfe4abf82c25 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -140,9 +140,10 @@ pub struct Arc<T: ?Sized> {
     _p: PhantomData<ArcInner<T>>,
 }
 
+#[doc(hidden)]
 #[pin_data]
 #[repr(C)]
-struct ArcInner<T: ?Sized> {
+pub struct ArcInner<T: ?Sized> {
     refcount: Opaque<bindings::refcount_t>,
     data: T,
 }
@@ -342,18 +343,20 @@ pub fn into_unique_or_drop(self) -> Option<Pin<UniqueArc<T>>> {
     }
 }
 
-impl<T: 'static> ForeignOwnable for Arc<T> {
+// SAFETY: The `into_foreign` function returns a pointer that is well-aligned.
+unsafe impl<T: 'static> ForeignOwnable for Arc<T> {
+    type PointedTo = ArcInner<T>;
     type Borrowed<'a> = ArcBorrow<'a, T>;
     type BorrowedMut<'a> = Self::Borrowed<'a>;
 
-    fn into_foreign(self) -> *mut crate::ffi::c_void {
-        ManuallyDrop::new(self).ptr.as_ptr().cast()
+    fn into_foreign(self) -> *mut Self::PointedTo {
+        ManuallyDrop::new(self).ptr.as_ptr()
     }
 
-    unsafe fn from_foreign(ptr: *mut crate::ffi::c_void) -> Self {
+    unsafe fn from_foreign(ptr: *mut Self::PointedTo) -> Self {
         // 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.cast::<ArcInner<T>>()) };
+        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
@@ -361,17 +364,17 @@ unsafe fn from_foreign(ptr: *mut crate::ffi::c_void) -> Self {
         unsafe { Self::from_inner(inner) }
     }
 
-    unsafe fn borrow<'a>(ptr: *mut crate::ffi::c_void) -> ArcBorrow<'a, T> {
+    unsafe fn borrow<'a>(ptr: *mut Self::PointedTo) -> ArcBorrow<'a, T> {
         // 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.cast::<ArcInner<T>>()) };
+        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) }
     }
 
-    unsafe fn borrow_mut<'a>(ptr: *mut crate::ffi::c_void) -> ArcBorrow<'a, T> {
+    unsafe fn borrow_mut<'a>(ptr: *mut Self::PointedTo) -> ArcBorrow<'a, T> {
         // SAFETY: The safety requirements for `borrow_mut` are a superset of the safety
         // requirements for `borrow`.
         unsafe { Self::borrow(ptr) }
diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 2bbaab83b9d6..55ddd50e8aaa 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -18,7 +18,19 @@
 ///
 /// This trait is meant to be used in cases when Rust objects are stored in C objects and
 /// eventually "freed" back to Rust.
-pub trait ForeignOwnable: Sized {
+///
+/// # Safety
+///
+/// Implementers must ensure that [`into_foreign`] returns a pointer which meets the alignment
+/// requirements of [`PointedTo`].
+///
+/// [`into_foreign`]: Self::into_foreign
+/// [`PointedTo`]: Self::PointedTo
+pub unsafe trait ForeignOwnable: Sized {
+    /// Type used when the value is foreign-owned. In practical terms only defines the alignment of
+    /// the pointer.
+    type PointedTo;
+
     /// Type used to immutably borrow a value that is currently foreign-owned.
     type Borrowed<'a>;
 
@@ -27,16 +39,18 @@ pub trait ForeignOwnable: Sized {
 
     /// Converts a Rust-owned object to a foreign-owned one.
     ///
-    /// The foreign representation is a pointer to void. There are no guarantees for this pointer.
-    /// For example, it might be invalid, dangling or pointing to uninitialized memory. Using it in
-    /// any way except for [`from_foreign`], [`try_from_foreign`], [`borrow`], or [`borrow_mut`] can
-    /// result in undefined behavior.
+    /// # Guarantees
+    ///
+    /// The return value is guaranteed to be well-aligned, but there are no other guarantees for
+    /// this pointer. For example, it might be null, dangling, or point to uninitialized memory.
+    /// Using it in any way except for [`ForeignOwnable::from_foreign`], [`ForeignOwnable::borrow`],
+    /// [`ForeignOwnable::try_from_foreign`] can result in undefined behavior.
     ///
     /// [`from_foreign`]: Self::from_foreign
     /// [`try_from_foreign`]: Self::try_from_foreign
     /// [`borrow`]: Self::borrow
     /// [`borrow_mut`]: Self::borrow_mut
-    fn into_foreign(self) -> *mut crate::ffi::c_void;
+    fn into_foreign(self) -> *mut Self::PointedTo;
 
     /// Converts a foreign-owned object back to a Rust-owned one.
     ///
@@ -46,7 +60,7 @@ pub trait ForeignOwnable: Sized {
     /// must not be passed to `from_foreign` more than once.
     ///
     /// [`into_foreign`]: Self::into_foreign
-    unsafe fn from_foreign(ptr: *mut crate::ffi::c_void) -> Self;
+    unsafe fn from_foreign(ptr: *mut Self::PointedTo) -> Self;
 
     /// Tries to convert a foreign-owned object back to a Rust-owned one.
     ///
@@ -58,7 +72,7 @@ pub trait ForeignOwnable: Sized {
     /// `ptr` must either be null or satisfy the safety requirements for [`from_foreign`].
     ///
     /// [`from_foreign`]: Self::from_foreign
-    unsafe fn try_from_foreign(ptr: *mut crate::ffi::c_void) -> Option<Self> {
+    unsafe fn try_from_foreign(ptr: *mut Self::PointedTo) -> Option<Self> {
         if ptr.is_null() {
             None
         } else {
@@ -81,7 +95,7 @@ unsafe fn try_from_foreign(ptr: *mut crate::ffi::c_void) -> Option<Self> {
     ///
     /// [`into_foreign`]: Self::into_foreign
     /// [`from_foreign`]: Self::from_foreign
-    unsafe fn borrow<'a>(ptr: *mut crate::ffi::c_void) -> Self::Borrowed<'a>;
+    unsafe fn borrow<'a>(ptr: *mut Self::PointedTo) -> Self::Borrowed<'a>;
 
     /// Borrows a foreign-owned object mutably.
     ///
@@ -109,21 +123,23 @@ unsafe fn try_from_foreign(ptr: *mut crate::ffi::c_void) -> Option<Self> {
     /// [`from_foreign`]: Self::from_foreign
     /// [`borrow`]: Self::borrow
     /// [`Arc`]: crate::sync::Arc
-    unsafe fn borrow_mut<'a>(ptr: *mut crate::ffi::c_void) -> Self::BorrowedMut<'a>;
+    unsafe fn borrow_mut<'a>(ptr: *mut Self::PointedTo) -> Self::BorrowedMut<'a>;
 }
 
-impl ForeignOwnable for () {
+// SAFETY: The `into_foreign` function returns a pointer that is dangling, but well-aligned.
+unsafe impl ForeignOwnable for () {
+    type PointedTo = ();
     type Borrowed<'a> = ();
     type BorrowedMut<'a> = ();
 
-    fn into_foreign(self) -> *mut crate::ffi::c_void {
+    fn into_foreign(self) -> *mut Self::PointedTo {
         core::ptr::NonNull::dangling().as_ptr()
     }
 
-    unsafe fn from_foreign(_: *mut crate::ffi::c_void) -> Self {}
+    unsafe fn from_foreign(_: *mut Self::PointedTo) -> Self {}
 
-    unsafe fn borrow<'a>(_: *mut crate::ffi::c_void) -> Self::Borrowed<'a> {}
-    unsafe fn borrow_mut<'a>(_: *mut crate::ffi::c_void) -> Self::BorrowedMut<'a> {}
+    unsafe fn borrow<'a>(_: *mut Self::PointedTo) -> Self::Borrowed<'a> {}
+    unsafe fn borrow_mut<'a>(_: *mut Self::PointedTo) -> Self::BorrowedMut<'a> {}
 }
 
 /// Runs a cleanup function/closure when dropped.

-- 
2.48.1

[PATCH v16 3/4] rust: xarray: Add an abstraction for XArray

[Tamir Duberstein]

`XArray` is an efficient sparse array of pointers. Add a Rust
abstraction for this type.

This implementation bounds the element type on `ForeignOwnable` and
requires explicit locking for all operations. Future work may leverage
RCU to enable lockless operation.

Inspired-by: Maíra Canal <mcanal@igalia.com>
Inspired-by: Asahi Lina <lina@asahilina.net>
Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 rust/bindings/bindings_helper.h |   6 +
 rust/helpers/helpers.c          |   1 +
 rust/helpers/xarray.c           |  28 ++++
 rust/kernel/alloc.rs            |   5 +
 rust/kernel/lib.rs              |   1 +
 rust/kernel/xarray.rs           | 276 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 317 insertions(+)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 55354e4dec14..249070b5abf9 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -34,6 +34,7 @@
 #include <linux/tracepoint.h>
 #include <linux/wait.h>
 #include <linux/workqueue.h>
+#include <linux/xarray.h>
 #include <trace/events/rust_sample.h>
 
 /* `bindgen` gets confused at certain things. */
@@ -47,3 +48,8 @@ const gfp_t RUST_CONST_HELPER___GFP_ZERO = __GFP_ZERO;
 const gfp_t RUST_CONST_HELPER___GFP_HIGHMEM = ___GFP_HIGHMEM;
 const gfp_t RUST_CONST_HELPER___GFP_NOWARN = ___GFP_NOWARN;
 const blk_features_t RUST_CONST_HELPER_BLK_FEAT_ROTATIONAL = BLK_FEAT_ROTATIONAL;
+
+const xa_mark_t RUST_CONST_HELPER_XA_PRESENT = XA_PRESENT;
+
+const gfp_t RUST_CONST_HELPER_XA_FLAGS_ALLOC = XA_FLAGS_ALLOC;
+const gfp_t RUST_CONST_HELPER_XA_FLAGS_ALLOC1 = XA_FLAGS_ALLOC1;
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 0640b7e115be..6811f71f2cbb 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -35,3 +35,4 @@
 #include "vmalloc.c"
 #include "wait.c"
 #include "workqueue.c"
+#include "xarray.c"
diff --git a/rust/helpers/xarray.c b/rust/helpers/xarray.c
new file mode 100644
index 000000000000..60b299f11451
--- /dev/null
+++ b/rust/helpers/xarray.c
@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/xarray.h>
+
+int rust_helper_xa_err(void *entry)
+{
+	return xa_err(entry);
+}
+
+void rust_helper_xa_init_flags(struct xarray *xa, gfp_t flags)
+{
+	return xa_init_flags(xa, flags);
+}
+
+int rust_helper_xa_trylock(struct xarray *xa)
+{
+	return xa_trylock(xa);
+}
+
+void rust_helper_xa_lock(struct xarray *xa)
+{
+	return xa_lock(xa);
+}
+
+void rust_helper_xa_unlock(struct xarray *xa)
+{
+	return xa_unlock(xa);
+}
diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
index fc9c9c41cd79..77840413598d 100644
--- a/rust/kernel/alloc.rs
+++ b/rust/kernel/alloc.rs
@@ -39,6 +39,11 @@
 pub struct Flags(u32);
 
 impl Flags {
+    /// Get a flags value with all bits unset.
+    pub fn empty() -> Self {
+        Self(0)
+    }
+
     /// Get the raw representation of this flag.
     pub(crate) fn as_raw(self) -> u32 {
         self.0
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 496ed32b0911..abe419362c73 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -84,6 +84,7 @@
 pub mod types;
 pub mod uaccess;
 pub mod workqueue;
+pub mod xarray;
 
 #[doc(hidden)]
 pub use bindings;
diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs
new file mode 100644
index 000000000000..8115dd7b4dd0
--- /dev/null
+++ b/rust/kernel/xarray.rs
@@ -0,0 +1,276 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! XArray abstraction.
+//!
+//! C header: [`include/linux/xarray.h`](srctree/include/linux/xarray.h)
+
+use crate::{
+    alloc, bindings, build_assert,
+    error::{Error, Result},
+    init::PinInit,
+    pin_init,
+    types::{ForeignOwnable, NotThreadSafe, Opaque},
+};
+use core::{iter, marker::PhantomData, mem, pin::Pin, ptr::NonNull};
+use macros::{pin_data, pinned_drop};
+
+/// An array which efficiently maps sparse integer indices to owned objects.
+///
+/// This is similar to a [`crate::alloc::kvec::Vec<Option<T>>`], but more efficient when there are
+/// holes in the index space, and can be efficiently grown.
+///
+/// # Invariants
+///
+/// `self.xa` is always an initialized and valid [`bindings::xarray`] whose entries are either
+/// `XA_ZERO_ENTRY` or came from `T::into_foreign`.
+///
+/// # Examples
+///
+/// ```rust
+/// use kernel::alloc::KBox;
+/// use kernel::xarray::{AllocKind, XArray};
+///
+/// let xa = KBox::pin_init(XArray::new(AllocKind::Alloc1), GFP_KERNEL)?;
+///
+/// let dead = KBox::new(0xdead, GFP_KERNEL)?;
+/// let beef = KBox::new(0xbeef, GFP_KERNEL)?;
+///
+/// let mut guard = xa.lock();
+///
+/// assert_eq!(guard.get(0), None);
+///
+/// assert_eq!(guard.store(0, dead, GFP_KERNEL)?.as_deref(), None);
+/// assert_eq!(guard.get(0).copied(), Some(0xdead));
+///
+/// *guard.get_mut(0).unwrap() = 0xffff;
+/// assert_eq!(guard.get(0).copied(), Some(0xffff));
+///
+/// assert_eq!(guard.store(0, beef, GFP_KERNEL)?.as_deref().copied(), Some(0xffff));
+/// assert_eq!(guard.get(0).copied(), Some(0xbeef));
+///
+/// guard.remove(0);
+/// assert_eq!(guard.get(0), None);
+///
+/// # Ok::<(), Error>(())
+/// ```
+#[pin_data(PinnedDrop)]
+pub struct XArray<T: ForeignOwnable> {
+    #[pin]
+    xa: Opaque<bindings::xarray>,
+    _p: PhantomData<T>,
+}
+
+#[pinned_drop]
+impl<T: ForeignOwnable> PinnedDrop for XArray<T> {
+    fn drop(self: Pin<&mut Self>) {
+        self.iter().for_each(|ptr| {
+            let ptr = ptr.as_ptr();
+            // SAFETY: `ptr` came from `T::into_foreign`.
+            //
+            // INVARIANT: we own the only reference to the array which is being dropped so the
+            // broken invariant is not observable on function exit.
+            drop(unsafe { T::from_foreign(ptr) })
+        });
+
+        // SAFETY: `self.xa` is always valid by the type invariant.
+        unsafe { bindings::xa_destroy(self.xa.get()) };
+    }
+}
+
+/// Flags passed to [`XArray::new`] to configure the array's allocation tracking behavior.
+pub enum AllocKind {
+    /// Consider the first element to be at index 0.
+    Alloc,
+    /// Consider the first element to be at index 1.
+    Alloc1,
+}
+
+impl<T: ForeignOwnable> XArray<T> {
+    /// Creates a new [`XArray`].
+    pub fn new(kind: AllocKind) -> impl PinInit<Self> {
+        let flags = match kind {
+            AllocKind::Alloc => bindings::XA_FLAGS_ALLOC,
+            AllocKind::Alloc1 => bindings::XA_FLAGS_ALLOC1,
+        };
+        pin_init!(Self {
+            // SAFETY: `xa` is valid while the closure is called.
+            xa <- Opaque::ffi_init(|xa| unsafe {
+                bindings::xa_init_flags(xa, flags)
+            }),
+            _p: PhantomData,
+        })
+    }
+
+    fn iter(&self) -> impl Iterator<Item = NonNull<T::PointedTo>> + '_ {
+        let mut index = 0;
+
+        // SAFETY: `self.xa` is always valid by the type invariant.
+        iter::once(unsafe {
+            bindings::xa_find(self.xa.get(), &mut index, usize::MAX, bindings::XA_PRESENT)
+        })
+        .chain(iter::from_fn(move || {
+            // SAFETY: `self.xa` is always valid by the type invariant.
+            Some(unsafe {
+                bindings::xa_find_after(self.xa.get(), &mut index, usize::MAX, bindings::XA_PRESENT)
+            })
+        }))
+        .map_while(|ptr| NonNull::new(ptr.cast()))
+    }
+
+    /// Attempts to lock the [`XArray`] for exclusive access.
+    pub fn try_lock(&self) -> Option<Guard<'_, T>> {
+        // SAFETY: `self.xa` is always valid by the type invariant.
+        if (unsafe { bindings::xa_trylock(self.xa.get()) } != 0) {
+            Some(Guard {
+                xa: self,
+                _not_send: NotThreadSafe,
+            })
+        } else {
+            None
+        }
+    }
+
+    /// Locks the [`XArray`] for exclusive access.
+    pub fn lock(&self) -> Guard<'_, T> {
+        // SAFETY: `self.xa` is always valid by the type invariant.
+        unsafe { bindings::xa_lock(self.xa.get()) };
+
+        Guard {
+            xa: self,
+            _not_send: NotThreadSafe,
+        }
+    }
+}
+
+/// A lock guard.
+///
+/// The lock is unlocked when the guard goes out of scope.
+#[must_use = "the lock unlocks immediately when the guard is unused"]
+pub struct Guard<'a, T: ForeignOwnable> {
+    xa: &'a XArray<T>,
+    _not_send: NotThreadSafe,
+}
+
+impl<T: ForeignOwnable> Drop for Guard<'_, T> {
+    fn drop(&mut self) {
+        // SAFETY:
+        // - `self.xa.xa` is always valid by the type invariant.
+        // - The caller holds the lock, so it is safe to unlock it.
+        unsafe { bindings::xa_unlock(self.xa.xa.get()) };
+    }
+}
+
+/// The error returned by [`store`](Guard::store).
+///
+/// Contains the underlying error and the value that was not stored.
+pub struct StoreError<T> {
+    /// The error that occurred.
+    pub error: Error,
+    /// The value that was not stored.
+    pub value: T,
+}
+
+impl<T> From<StoreError<T>> for Error {
+    fn from(value: StoreError<T>) -> Self {
+        let StoreError { error, value: _ } = value;
+        error
+    }
+}
+
+impl<'a, T: ForeignOwnable> Guard<'a, T> {
+    fn load<F, U>(&self, index: usize, f: F) -> Option<U>
+    where
+        F: FnOnce(NonNull<T::PointedTo>) -> U,
+    {
+        // SAFETY: `self.xa.xa` is always valid by the type invariant.
+        let ptr = unsafe { bindings::xa_load(self.xa.xa.get(), index) };
+        let ptr = NonNull::new(ptr.cast())?;
+        Some(f(ptr))
+    }
+
+    /// Provides a reference to the element at the given index.
+    pub fn get(&self, index: usize) -> Option<T::Borrowed<'_>> {
+        self.load(index, |ptr| {
+            // SAFETY: `ptr` came from `T::into_foreign`.
+            unsafe { T::borrow(ptr.as_ptr()) }
+        })
+    }
+
+    /// Provides a mutable reference to the element at the given index.
+    pub fn get_mut(&mut self, index: usize) -> Option<T::BorrowedMut<'_>> {
+        self.load(index, |ptr| {
+            // SAFETY: `ptr` came from `T::into_foreign`.
+            unsafe { T::borrow_mut(ptr.as_ptr()) }
+        })
+    }
+
+    /// Removes and returns the element at the given index.
+    pub fn remove(&mut self, index: usize) -> Option<T> {
+        // SAFETY: `self.xa.xa` is always valid by the type invariant.
+        //
+        // SAFETY: The caller holds the lock.
+        let ptr = unsafe { bindings::__xa_erase(self.xa.xa.get(), index) }.cast();
+        // SAFETY: `ptr` is either NULL or came from `T::into_foreign`.
+        //
+        // SAFETY: `&mut self` guarantees that the lifetimes of [`T::Borrowed`] and
+        // [`T::BorrowedMut`] borrowed from `self` have ended.
+        unsafe { T::try_from_foreign(ptr) }
+    }
+
+    /// Stores an element at the given index.
+    ///
+    /// May drop the lock if needed to allocate memory, and then reacquire it afterwards.
+    ///
+    /// On success, returns the element which was previously at the given index.
+    ///
+    /// On failure, returns the element which was attempted to be stored.
+    pub fn store(
+        &mut self,
+        index: usize,
+        value: T,
+        gfp: alloc::Flags,
+    ) -> Result<Option<T>, StoreError<T>> {
+        build_assert!(
+            mem::align_of::<T::PointedTo>() >= 4,
+            "pointers stored in XArray must be 4-byte aligned"
+        );
+        let new = value.into_foreign();
+
+        let old = {
+            let new = new.cast();
+            // SAFETY: `self.xa.xa` is always valid by the type invariant.
+            //
+            // SAFETY: The caller holds the lock.
+            //
+            // INVARIANT: `new` came from `T::into_foreign`.
+            unsafe { bindings::__xa_store(self.xa.xa.get(), index, new, gfp.as_raw()) }
+        };
+
+        // SAFETY: `__xa_store` returns the old entry at this index on success or `xa_err` if an
+        // error happened.
+        let errno = unsafe { bindings::xa_err(old) };
+        if errno != 0 {
+            // SAFETY: `new` came from `T::into_foreign` and `__xa_store` does not take
+            // ownership of the value on error.
+            let value = unsafe { T::from_foreign(new) };
+            Err(StoreError {
+                value,
+                error: Error::from_errno(errno),
+            })
+        } else {
+            let old = old.cast();
+            // SAFETY: `ptr` is either NULL or came from `T::into_foreign`.
+            //
+            // NB: `XA_ZERO_ENTRY` is never returned by functions belonging to the Normal XArray
+            // API; such entries present as `NULL`.
+            Ok(unsafe { T::try_from_foreign(old) })
+        }
+    }
+}
+
+// SAFETY: `XArray<T>` has no shared mutable state so it is `Send` iff `T` is `Send`.
+unsafe impl<T: ForeignOwnable + Send> Send for XArray<T> {}
+
+// SAFETY: `XArray<T>` serialises the interior mutability it provides so it is `Sync` iff `T` is
+// `Send`.
+unsafe impl<T: ForeignOwnable + Send> Sync for XArray<T> {}

-- 
2.48.1

[PATCH v16 4/4] MAINTAINERS: add entry for Rust XArray API

[Tamir Duberstein]

Add an entry for the Rust xarray abstractions.

Acked-by: Andreas Hindborg <a.hindborg@kernel.org>
Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 MAINTAINERS | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 896a307fa065..88282b6e689b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25749,6 +25749,17 @@ F:	lib/test_xarray.c
 F:	lib/xarray.c
 F:	tools/testing/radix-tree
 
+XARRAY API [RUST]
+M:	Tamir Duberstein <tamird@gmail.com>
+M:	Andreas Hindborg <a.hindborg@kernel.org>
+L:	rust-for-linux@vger.kernel.org
+S:	Supported
+W:	https://rust-for-linux.com
+B:	https://github.com/Rust-for-Linux/linux/issues
+C:	https://rust-for-linux.zulipchat.com
+T:	git https://github.com/Rust-for-Linux/linux.git rust-next
+F:	rust/kernel/xarray.rs
+
 XBOX DVD IR REMOTE
 M:	Benjamin Valentin <benpicco@googlemail.com>
 S:	Maintained

-- 
2.48.1

Re: [PATCH v16 2/4] rust: types: add `ForeignOwnable::PointedTo`

[Benno Lossin]

On 07.02.25 14:58, Tamir Duberstein wrote:
> Allow implementors to specify the foreign pointer type; this exposes
> information about the pointed-to type such as its alignment.
> 
> This requires the trait to be `unsafe` since it is now possible for
> implementors to break soundness by returning a misaligned pointer.
> 
> Encoding the pointer type in the trait (and avoiding pointer casts)
> allows the compiler to check that implementors return the correct
> pointer type. This is preferable to directly encoding the alignment in
> the trait using a constant as the compiler would be unable to check it.
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> Reviewed-by: Fiona Behrens <me@kloenk.dev>
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> ---
>  rust/kernel/alloc/kbox.rs | 38 ++++++++++++++++++++------------------
>  rust/kernel/miscdevice.rs |  7 ++++++-
>  rust/kernel/pci.rs        |  2 ++
>  rust/kernel/platform.rs   |  2 ++
>  rust/kernel/sync/arc.rs   | 21 ++++++++++++---------
>  rust/kernel/types.rs      | 46 +++++++++++++++++++++++++++++++---------------
>  6 files changed, 73 insertions(+), 43 deletions(-)

When compiling this (on top of rust-next), I get the following error:

    error[E0308]: mismatched types
       --> rust/kernel/miscdevice.rs:300:62
        |
    300 |     let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
        |                           ---------------------------------- ^^^^^^^ expected `*mut <<T as MiscDevice>::Ptr as ForeignOwnable>::PointedTo`, found `*mut c_void`
        |                           |
        |                           arguments to this function are incorrect
        |
        = note: expected raw pointer `*mut <<T as MiscDevice>::Ptr as ForeignOwnable>::PointedTo`
                   found raw pointer `*mut c_void`
        = help: consider constraining the associated type `<<T as MiscDevice>::Ptr as ForeignOwnable>::PointedTo` to `c_void` or calling a method that returns `<<T as MiscDevice>::Ptr as ForeignOwnable>::PointedTo`
        = note: for more information, visit https://doc.rust-lang.org/book/ch19-03-advanced-traits.html
    note: associated function defined here
       --> rust/kernel/types.rs:98:15
        |
    98  |     unsafe fn borrow<'a>(ptr: *mut Self::PointedTo) -> Self::Borrowed<'a>;
        |               ^^^^^^
    
    error: aborting due to 1 previous error

Can anyone reproduce?

---
Cheers,
Benno

Re: [PATCH v16 2/4] rust: types: add `ForeignOwnable::PointedTo`

[Tamir Duberstein]

On Sun, Feb 16, 2025 at 8:39 PM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On 07.02.25 14:58, Tamir Duberstein wrote:
> > Allow implementors to specify the foreign pointer type; this exposes
> > information about the pointed-to type such as its alignment.
> >
> > This requires the trait to be `unsafe` since it is now possible for
> > implementors to break soundness by returning a misaligned pointer.
> >
> > Encoding the pointer type in the trait (and avoiding pointer casts)
> > allows the compiler to check that implementors return the correct
> > pointer type. This is preferable to directly encoding the alignment in
> > the trait using a constant as the compiler would be unable to check it.
> >
> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> > Reviewed-by: Fiona Behrens <me@kloenk.dev>
> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> > ---
> >  rust/kernel/alloc/kbox.rs | 38 ++++++++++++++++++++------------------
> >  rust/kernel/miscdevice.rs |  7 ++++++-
> >  rust/kernel/pci.rs        |  2 ++
> >  rust/kernel/platform.rs   |  2 ++
> >  rust/kernel/sync/arc.rs   | 21 ++++++++++++---------
> >  rust/kernel/types.rs      | 46 +++++++++++++++++++++++++++++++---------------
> >  6 files changed, 73 insertions(+), 43 deletions(-)
>
> When compiling this (on top of rust-next), I get the following error:
>
>     error[E0308]: mismatched types
>        --> rust/kernel/miscdevice.rs:300:62
>         |
>     300 |     let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
>         |                           ---------------------------------- ^^^^^^^ expected `*mut <<T as MiscDevice>::Ptr as ForeignOwnable>::PointedTo`, found `*mut c_void`
>         |                           |
>         |                           arguments to this function are incorrect
>         |
>         = note: expected raw pointer `*mut <<T as MiscDevice>::Ptr as ForeignOwnable>::PointedTo`
>                    found raw pointer `*mut c_void`
>         = help: consider constraining the associated type `<<T as MiscDevice>::Ptr as ForeignOwnable>::PointedTo` to `c_void` or calling a method that returns `<<T as MiscDevice>::Ptr as ForeignOwnable>::PointedTo`
>         = note: for more information, visit https://doc.rust-lang.org/book/ch19-03-advanced-traits.html
>     note: associated function defined here
>        --> rust/kernel/types.rs:98:15
>         |
>     98  |     unsafe fn borrow<'a>(ptr: *mut Self::PointedTo) -> Self::Borrowed<'a>;
>         |               ^^^^^^
>
>     error: aborting due to 1 previous error
>
> Can anyone reproduce?
>
> ---
> Cheers,
> Benno
>

Looks like this code is behind #[cfg(CONFIG_COMPAT)] and I missed
updating it. It is fixed by

diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
index f1a081dd64c7..004dc87f441f 100644
--- a/rust/kernel/miscdevice.rs
+++ b/rust/kernel/miscdevice.rs
@@ -296,6 +296,7 @@ impl<T: MiscDevice> VtableHelper<T> {
 ) -> c_long {
     // SAFETY: The compat ioctl call of a file can access the private data.
     let private = unsafe { (*file).private_data };
+    let private = private.cast();
     // SAFETY: Ioctl calls can borrow the private data of the file.
     let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };

I'll include that in the next version.

Re: [PATCH v16 1/4] rust: remove redundant `as _` casts

[Danilo Krummrich]

On Fri, Feb 07, 2025 at 08:58:24AM -0500, Tamir Duberstein wrote:
> Remove redundant casts added in commit 1bd8b6b2c5d3 ("rust: pci: add
> basic PCI device / driver abstractions") and commit 683a63befc73 ("rust:
> platform: add basic platform device / driver abstractions")

I thought of doing it the other way around: Do only the *required* changes in
commit "rust: types: add `ForeignOwnable::PointedTo`", i.e. no need to touch
this code at all. And then switch to cast() in a subsequent patch.

This way you don't need to remove (previously unnecessary) casts and then add
them back in.

> 
> While I'm churning this line, move the `.into_foreign()` call to its own
> statement to avoid churn in the next commit which adds a `.cast()` call.
> 
> Fixes: 1bd8b6b2c5d3 ("rust: pci: add basic PCI device / driver abstractions")
> Fixes: 683a63befc73 ("rust: platform: add basic platform device / driver abstractions")

No, at the time those casts were indeed necessary, because the types differed in
mutability.

"A Fixes: tag indicates that the patch fixes an issue in a previous commit." [1]

Even if the cast was unnecessary in the first place, it is at least questionable
to me whether this falls under "fixing an issue".

[1] https://docs.kernel.org/process/submitting-patches.html#reviewer-s-statement-of-oversight

> Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> ---
>  rust/kernel/pci.rs      | 3 ++-
>  rust/kernel/platform.rs | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 4c98b5b9aa1e..6c3bc14b42ad 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -72,10 +72,11 @@ extern "C" fn probe_callback(
>  
>          match T::probe(&mut pdev, info) {
>              Ok(data) => {
> +                let data = data.into_foreign();
>                  // Let the `struct pci_dev` own a reference of the driver's private data.
>                  // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
>                  // `struct pci_dev`.
> -                unsafe { bindings::pci_set_drvdata(pdev.as_raw(), data.into_foreign() as _) };
> +                unsafe { bindings::pci_set_drvdata(pdev.as_raw(), data) };

Please do not factor this out, it is unnecessary for what you want to accomplish
with your commit.

>              }
>              Err(err) => return Error::to_errno(err),
>          }
> diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
> index 50e6b0421813..dea104563fa9 100644
> --- a/rust/kernel/platform.rs
> +++ b/rust/kernel/platform.rs
> @@ -63,10 +63,11 @@ extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> kernel::ff
>          let info = <Self as driver::Adapter>::id_info(pdev.as_ref());
>          match T::probe(&mut pdev, info) {
>              Ok(data) => {
> +                let data = data.into_foreign();
>                  // Let the `struct platform_device` own a reference of the driver's private data.
>                  // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
>                  // `struct platform_device`.
> -                unsafe { bindings::platform_set_drvdata(pdev.as_raw(), data.into_foreign() as _) };
> +                unsafe { bindings::platform_set_drvdata(pdev.as_raw(), data) };
>              }
>              Err(err) => return Error::to_errno(err),
>          }
> 
> -- 
> 2.48.1
> 

Re: [PATCH v16 3/4] rust: xarray: Add an abstraction for XArray

[Danilo Krummrich]

On Fri, Feb 07, 2025 at 08:58:26AM -0500, Tamir Duberstein wrote:
> `XArray` is an efficient sparse array of pointers. Add a Rust
> abstraction for this type.
> 
> This implementation bounds the element type on `ForeignOwnable` and
> requires explicit locking for all operations. Future work may leverage
> RCU to enable lockless operation.
> 
> Inspired-by: Maíra Canal <mcanal@igalia.com>
> Inspired-by: Asahi Lina <lina@asahilina.net>
> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> ---
>  rust/bindings/bindings_helper.h |   6 +
>  rust/helpers/helpers.c          |   1 +
>  rust/helpers/xarray.c           |  28 ++++
>  rust/kernel/alloc.rs            |   5 +
>  rust/kernel/lib.rs              |   1 +
>  rust/kernel/xarray.rs           | 276 ++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 317 insertions(+)
> 
> diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
> index fc9c9c41cd79..77840413598d 100644
> --- a/rust/kernel/alloc.rs
> +++ b/rust/kernel/alloc.rs
> @@ -39,6 +39,11 @@
>  pub struct Flags(u32);
>  
>  impl Flags {
> +    /// Get a flags value with all bits unset.
> +    pub fn empty() -> Self {
> +        Self(0)
> +    }

No! Zero is not a reasonable default for GFP flags. In fact, I don't know any
place in the kernel where we would want no reclaim + no IO + no FS without any
other flags (such as high-priority or kswapd can wake). Especially, because for
NOIO and NOFS, memalloc_noio_{save, restore} and memalloc_nofs_{save, restore}
guards should be used instead.

You also don't seem to use this anywhere anyways.

Please also make sure to not bury such changes in unrelated other patches.

> +/// The error returned by [`store`](Guard::store).
> +///
> +/// Contains the underlying error and the value that was not stored.
> +pub struct StoreError<T> {
> +    /// The error that occurred.
> +    pub error: Error,
> +    /// The value that was not stored.
> +    pub value: T,
> +}
> +
> +impl<T> From<StoreError<T>> for Error {
> +    fn from(value: StoreError<T>) -> Self {
> +        let StoreError { error, value: _ } = value;
> +        error

Why not just `value.error`?

Re: [PATCH v16 2/4] rust: types: add `ForeignOwnable::PointedTo`

[Danilo Krummrich]

On Fri, Feb 07, 2025 at 08:58:25AM -0500, Tamir Duberstein wrote:
> Allow implementors to specify the foreign pointer type; this exposes
> information about the pointed-to type such as its alignment.
> 
> This requires the trait to be `unsafe` since it is now possible for
> implementors to break soundness by returning a misaligned pointer.
> 
> Encoding the pointer type in the trait (and avoiding pointer casts)
> allows the compiler to check that implementors return the correct
> pointer type. This is preferable to directly encoding the alignment in
> the trait using a constant as the compiler would be unable to check it.
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> Reviewed-by: Fiona Behrens <me@kloenk.dev>

I know that Andreas also asked you to pick up the RBs from [1], but - without
speaking for any of the people above - given that you changed this commit after
you received all those RBs you should also consider dropping them. Especially,
since you do not mention the changes you did for this commit in the version
history.

Just to be clear, often it is also fine to keep tags for minor changes, but then
you should make people aware of them in the version history, such that they get
the chance to double check.

[1] https://lore.kernel.org/rust-for-linux/20250131-configfs-v1-1-87947611401c@kernel.org/

> Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> ---
>  rust/kernel/alloc/kbox.rs | 38 ++++++++++++++++++++------------------
>  rust/kernel/miscdevice.rs |  7 ++++++-
>  rust/kernel/pci.rs        |  2 ++
>  rust/kernel/platform.rs   |  2 ++
>  rust/kernel/sync/arc.rs   | 21 ++++++++++++---------
>  rust/kernel/types.rs      | 46 +++++++++++++++++++++++++++++++---------------
>  6 files changed, 73 insertions(+), 43 deletions(-)
> 
> diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> index e14433b2ab9d..f1a081dd64c7 100644
> --- a/rust/kernel/miscdevice.rs
> +++ b/rust/kernel/miscdevice.rs
> @@ -225,13 +225,15 @@ impl<T: MiscDevice> VtableHelper<T> {
>          Ok(ptr) => ptr,
>          Err(err) => return err.to_errno(),
>      };
> +    let ptr = ptr.into_foreign();
> +    let ptr = ptr.cast();

I still think that it's unnecessary to factor this out, you can just do it
inline as you did in previous versions of this patch and as this code did
before.

>  
>      // This overwrites the private data with the value specified by the user, changing the type of
>      // this file's private data. All future accesses to the private data is performed by other
>      // fops_* methods in this file, which all correctly cast the private data to the new type.
>      //
>      // SAFETY: The open call of a file can access the private data.
> -    unsafe { (*raw_file).private_data = ptr.into_foreign() };
> +    unsafe { (*raw_file).private_data = ptr };
>  
>      0
>  }
> @@ -246,6 +248,7 @@ impl<T: MiscDevice> VtableHelper<T> {
>  ) -> c_int {
>      // SAFETY: The release call of a file owns the private data.
>      let private = unsafe { (*file).private_data };
> +    let private = private.cast();
>      // SAFETY: The release call of a file owns the private data.
>      let ptr = unsafe { <T::Ptr as ForeignOwnable>::from_foreign(private) };
>  
> @@ -267,6 +270,7 @@ impl<T: MiscDevice> VtableHelper<T> {
>  ) -> c_long {
>      // SAFETY: The ioctl call of a file can access the private data.
>      let private = unsafe { (*file).private_data };
> +    let private = private.cast();
>      // SAFETY: Ioctl calls can borrow the private data of the file.
>      let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
>  
> @@ -316,6 +320,7 @@ impl<T: MiscDevice> VtableHelper<T> {
>  ) {
>      // SAFETY: The release call of a file owns the private data.
>      let private = unsafe { (*file).private_data };
> +    let private = private.cast();
>      // SAFETY: Ioctl calls can borrow the private data of the file.
>      let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
>      // SAFETY:
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 6c3bc14b42ad..eb25fabbff9c 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -73,6 +73,7 @@ extern "C" fn probe_callback(
>          match T::probe(&mut pdev, info) {
>              Ok(data) => {
>                  let data = data.into_foreign();
> +                let data = data.cast();

Same here and below, see also [2].

I understand you like this style and I'm not saying it's wrong or forbidden and
for code that you maintain such nits are entirely up to you as far as I'm
concerned.

But I also don't think there is a necessity to convert things to your preference
wherever you touch existing code.

I already explicitly asked you not to do so in [3] and yet you did so while
keeping my ACK. :(

(Only saying the latter for reference, no need to send a new version of [3],
otherwise I would have replied.)

[2] https://lore.kernel.org/rust-for-linux/Z7MYNQgo28sr_4RS@cassiopeiae/
[3] https://lore.kernel.org/rust-for-linux/20250213-aligned-alloc-v7-1-d2a2d0be164b@gmail.com/

>                  // Let the `struct pci_dev` own a reference of the driver's private data.
>                  // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
>                  // `struct pci_dev`.
> @@ -88,6 +89,7 @@ extern "C" fn remove_callback(pdev: *mut bindings::pci_dev) {
>          // SAFETY: The PCI bus only ever calls the remove callback with a valid pointer to a
>          // `struct pci_dev`.
>          let ptr = unsafe { bindings::pci_get_drvdata(pdev) };
> +        let ptr = ptr.cast();
>  
>          // SAFETY: `remove_callback` is only ever called after a successful call to
>          // `probe_callback`, hence it's guaranteed that `ptr` points to a valid and initialized
> diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
> index dea104563fa9..53764cb7f804 100644
> --- a/rust/kernel/platform.rs
> +++ b/rust/kernel/platform.rs
> @@ -64,6 +64,7 @@ extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> kernel::ff
>          match T::probe(&mut pdev, info) {
>              Ok(data) => {
>                  let data = data.into_foreign();
> +                let data = data.cast();
>                  // Let the `struct platform_device` own a reference of the driver's private data.
>                  // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
>                  // `struct platform_device`.
> @@ -78,6 +79,7 @@ extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> kernel::ff
>      extern "C" fn remove_callback(pdev: *mut bindings::platform_device) {
>          // SAFETY: `pdev` is a valid pointer to a `struct platform_device`.
>          let ptr = unsafe { bindings::platform_get_drvdata(pdev) };
> +        let ptr = ptr.cast();
>  
>          // SAFETY: `remove_callback` is only ever called after a successful call to
>          // `probe_callback`, hence it's guaranteed that `ptr` points to a valid and initialized

Re: [PATCH v16 3/4] rust: xarray: Add an abstraction for XArray

[Tamir Duberstein]

On Mon, Feb 17, 2025 at 6:35 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Fri, Feb 07, 2025 at 08:58:26AM -0500, Tamir Duberstein wrote:
> > `XArray` is an efficient sparse array of pointers. Add a Rust
> > abstraction for this type.
> >
> > This implementation bounds the element type on `ForeignOwnable` and
> > requires explicit locking for all operations. Future work may leverage
> > RCU to enable lockless operation.
> >
> > Inspired-by: Maíra Canal <mcanal@igalia.com>
> > Inspired-by: Asahi Lina <lina@asahilina.net>
> > Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> > ---
> >  rust/bindings/bindings_helper.h |   6 +
> >  rust/helpers/helpers.c          |   1 +
> >  rust/helpers/xarray.c           |  28 ++++
> >  rust/kernel/alloc.rs            |   5 +
> >  rust/kernel/lib.rs              |   1 +
> >  rust/kernel/xarray.rs           | 276 ++++++++++++++++++++++++++++++++++++++++
> >  6 files changed, 317 insertions(+)
> >
> > diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
> > index fc9c9c41cd79..77840413598d 100644
> > --- a/rust/kernel/alloc.rs
> > +++ b/rust/kernel/alloc.rs
> > @@ -39,6 +39,11 @@
> >  pub struct Flags(u32);
> >
> >  impl Flags {
> > +    /// Get a flags value with all bits unset.
> > +    pub fn empty() -> Self {
> > +        Self(0)
> > +    }
>
> No! Zero is not a reasonable default for GFP flags.

This is not a default.

> In fact, I don't know any
> place in the kernel where we would want no reclaim + no IO + no FS without any
> other flags (such as high-priority or kswapd can wake). Especially, because for
> NOIO and NOFS, memalloc_noio_{save, restore} and memalloc_nofs_{save, restore}
> guards should be used instead.
>
> You also don't seem to use this anywhere anyways.

This was used in an earlier iteration that included support for
reservations. I used this value when fulfilling a reservation because
it was an invariant of the API that no allocation would take place.

> Please also make sure to not bury such changes in unrelated other patches.

Thank you for spotting this errant change. Please consider whether it
serves anyone's purpose to accuse someone of underhanded behavior.

> > +/// The error returned by [`store`](Guard::store).
> > +///
> > +/// Contains the underlying error and the value that was not stored.
> > +pub struct StoreError<T> {
> > +    /// The error that occurred.
> > +    pub error: Error,
> > +    /// The value that was not stored.
> > +    pub value: T,
> > +}
> > +
> > +impl<T> From<StoreError<T>> for Error {
> > +    fn from(value: StoreError<T>) -> Self {
> > +        let StoreError { error, value: _ } = value;
> > +        error
>
> Why not just `value.error`?

I prefer the clarity that this results in the value being dropped. Is
there written guidance on this matter?

Re: [PATCH v16 3/4] rust: xarray: Add an abstraction for XArray

[Danilo Krummrich]

On Mon, Feb 17, 2025 at 08:43:12AM -0500, Tamir Duberstein wrote:
> On Mon, Feb 17, 2025 at 6:35 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Fri, Feb 07, 2025 at 08:58:26AM -0500, Tamir Duberstein wrote:
> > > `XArray` is an efficient sparse array of pointers. Add a Rust
> > > abstraction for this type.
> > >
> > > This implementation bounds the element type on `ForeignOwnable` and
> > > requires explicit locking for all operations. Future work may leverage
> > > RCU to enable lockless operation.
> > >
> > > Inspired-by: Maíra Canal <mcanal@igalia.com>
> > > Inspired-by: Asahi Lina <lina@asahilina.net>
> > > Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> > > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> > > ---
> > >  rust/bindings/bindings_helper.h |   6 +
> > >  rust/helpers/helpers.c          |   1 +
> > >  rust/helpers/xarray.c           |  28 ++++
> > >  rust/kernel/alloc.rs            |   5 +
> > >  rust/kernel/lib.rs              |   1 +
> > >  rust/kernel/xarray.rs           | 276 ++++++++++++++++++++++++++++++++++++++++
> > >  6 files changed, 317 insertions(+)
> > >
> > > diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
> > > index fc9c9c41cd79..77840413598d 100644
> > > --- a/rust/kernel/alloc.rs
> > > +++ b/rust/kernel/alloc.rs
> > > @@ -39,6 +39,11 @@
> > >  pub struct Flags(u32);
> > >
> > >  impl Flags {
> > > +    /// Get a flags value with all bits unset.
> > > +    pub fn empty() -> Self {
> > > +        Self(0)
> > > +    }
> >
> > No! Zero is not a reasonable default for GFP flags.
> 
> This is not a default.
> 
> > In fact, I don't know any
> > place in the kernel where we would want no reclaim + no IO + no FS without any
> > other flags (such as high-priority or kswapd can wake). Especially, because for
> > NOIO and NOFS, memalloc_noio_{save, restore} and memalloc_nofs_{save, restore}
> > guards should be used instead.
> >
> > You also don't seem to use this anywhere anyways.
> 
> This was used in an earlier iteration that included support for
> reservations. I used this value when fulfilling a reservation because
> it was an invariant of the API that no allocation would take place.
> 
> > Please also make sure to not bury such changes in unrelated other patches.
> 
> Thank you for spotting this errant change. Please consider whether it
> serves anyone's purpose to accuse someone of underhanded behavior.

As far as I can see I did not accuse anyone of underhanded behavior. But if it
came across this way to you, that wasn't the intention.

> 
> > > +/// The error returned by [`store`](Guard::store).
> > > +///
> > > +/// Contains the underlying error and the value that was not stored.
> > > +pub struct StoreError<T> {
> > > +    /// The error that occurred.
> > > +    pub error: Error,
> > > +    /// The value that was not stored.
> > > +    pub value: T,
> > > +}
> > > +
> > > +impl<T> From<StoreError<T>> for Error {
> > > +    fn from(value: StoreError<T>) -> Self {
> > > +        let StoreError { error, value: _ } = value;
> > > +        error
> >
> > Why not just `value.error`?
> 
> I prefer the clarity that this results in the value being dropped.

I don't think that any further clarity than the fact that value was passed by
value is needed.

Otherwise one could probably argue the same way for this:

fn from(value: StoreError<T>) -> Self {
   let error = value.error;
   drop(value);
   error
}

But that's up to you.

> Is there written guidance on this matter?

I don't think so.

Re: [PATCH v16 2/4] rust: types: add `ForeignOwnable::PointedTo`

[Tamir Duberstein]

On Mon, Feb 17, 2025 at 7:13 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Fri, Feb 07, 2025 at 08:58:25AM -0500, Tamir Duberstein wrote:
> > Allow implementors to specify the foreign pointer type; this exposes
> > information about the pointed-to type such as its alignment.
> >
> > This requires the trait to be `unsafe` since it is now possible for
> > implementors to break soundness by returning a misaligned pointer.
> >
> > Encoding the pointer type in the trait (and avoiding pointer casts)
> > allows the compiler to check that implementors return the correct
> > pointer type. This is preferable to directly encoding the alignment in
> > the trait using a constant as the compiler would be unable to check it.
> >
> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> > Reviewed-by: Fiona Behrens <me@kloenk.dev>
>
> I know that Andreas also asked you to pick up the RBs from [1], but - without
> speaking for any of the people above - given that you changed this commit after
> you received all those RBs you should also consider dropping them. Especially,
> since you do not mention the changes you did for this commit in the version
> history.
>
> Just to be clear, often it is also fine to keep tags for minor changes, but then
> you should make people aware of them in the version history, such that they get
> the chance to double check.

I will drop their RBs.

> [1] https://lore.kernel.org/rust-for-linux/20250131-configfs-v1-1-87947611401c@kernel.org/
>
> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> > ---
> >  rust/kernel/alloc/kbox.rs | 38 ++++++++++++++++++++------------------
> >  rust/kernel/miscdevice.rs |  7 ++++++-
> >  rust/kernel/pci.rs        |  2 ++
> >  rust/kernel/platform.rs   |  2 ++
> >  rust/kernel/sync/arc.rs   | 21 ++++++++++++---------
> >  rust/kernel/types.rs      | 46 +++++++++++++++++++++++++++++++---------------
> >  6 files changed, 73 insertions(+), 43 deletions(-)
> >
> > diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> > index e14433b2ab9d..f1a081dd64c7 100644
> > --- a/rust/kernel/miscdevice.rs
> > +++ b/rust/kernel/miscdevice.rs
> > @@ -225,13 +225,15 @@ impl<T: MiscDevice> VtableHelper<T> {
> >          Ok(ptr) => ptr,
> >          Err(err) => return err.to_errno(),
> >      };
> > +    let ptr = ptr.into_foreign();
> > +    let ptr = ptr.cast();
>
> I still think that it's unnecessary to factor this out, you can just do it
> inline as you did in previous versions of this patch and as this code did
> before.

I will do as you ask here.

>
> >
> >      // This overwrites the private data with the value specified by the user, changing the type of
> >      // this file's private data. All future accesses to the private data is performed by other
> >      // fops_* methods in this file, which all correctly cast the private data to the new type.
> >      //
> >      // SAFETY: The open call of a file can access the private data.
> > -    unsafe { (*raw_file).private_data = ptr.into_foreign() };
> > +    unsafe { (*raw_file).private_data = ptr };
> >
> >      0
> >  }
> > @@ -246,6 +248,7 @@ impl<T: MiscDevice> VtableHelper<T> {
> >  ) -> c_int {
> >      // SAFETY: The release call of a file owns the private data.
> >      let private = unsafe { (*file).private_data };
> > +    let private = private.cast();
> >      // SAFETY: The release call of a file owns the private data.
> >      let ptr = unsafe { <T::Ptr as ForeignOwnable>::from_foreign(private) };
> >
> > @@ -267,6 +270,7 @@ impl<T: MiscDevice> VtableHelper<T> {
> >  ) -> c_long {
> >      // SAFETY: The ioctl call of a file can access the private data.
> >      let private = unsafe { (*file).private_data };
> > +    let private = private.cast();
> >      // SAFETY: Ioctl calls can borrow the private data of the file.
> >      let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
> >
> > @@ -316,6 +320,7 @@ impl<T: MiscDevice> VtableHelper<T> {
> >  ) {
> >      // SAFETY: The release call of a file owns the private data.
> >      let private = unsafe { (*file).private_data };
> > +    let private = private.cast();
> >      // SAFETY: Ioctl calls can borrow the private data of the file.
> >      let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
> >      // SAFETY:
> > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> > index 6c3bc14b42ad..eb25fabbff9c 100644
> > --- a/rust/kernel/pci.rs
> > +++ b/rust/kernel/pci.rs
> > @@ -73,6 +73,7 @@ extern "C" fn probe_callback(
> >          match T::probe(&mut pdev, info) {
> >              Ok(data) => {
> >                  let data = data.into_foreign();
> > +                let data = data.cast();
>
> Same here and below, see also [2].

You're the maintainer, so I'll do what you ask here as well. I did it
this way because it avoids shadowing the git history with this change,
which I thought was the dominant preference.

> I understand you like this style and I'm not saying it's wrong or forbidden and
> for code that you maintain such nits are entirely up to you as far as I'm
> concerned.
>
> But I also don't think there is a necessity to convert things to your preference
> wherever you touch existing code.

This isn't a conversion, it's a choice made specifically to avoid
touching code that doesn't need to be touched (in this instance).

> I already explicitly asked you not to do so in [3] and yet you did so while
> keeping my ACK. :(
>
> (Only saying the latter for reference, no need to send a new version of [3],
> otherwise I would have replied.)
>
> [2] https://lore.kernel.org/rust-for-linux/Z7MYNQgo28sr_4RS@cassiopeiae/
> [3] https://lore.kernel.org/rust-for-linux/20250213-aligned-alloc-v7-1-d2a2d0be164b@gmail.com/

I will drop [2] and leave the `as _` casts in place to minimize
controversy here.

> >                  // Let the `struct pci_dev` own a reference of the driver's private data.
> >                  // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
> >                  // `struct pci_dev`.
> > @@ -88,6 +89,7 @@ extern "C" fn remove_callback(pdev: *mut bindings::pci_dev) {
> >          // SAFETY: The PCI bus only ever calls the remove callback with a valid pointer to a
> >          // `struct pci_dev`.
> >          let ptr = unsafe { bindings::pci_get_drvdata(pdev) };
> > +        let ptr = ptr.cast();
> >
> >          // SAFETY: `remove_callback` is only ever called after a successful call to
> >          // `probe_callback`, hence it's guaranteed that `ptr` points to a valid and initialized
> > diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
> > index dea104563fa9..53764cb7f804 100644
> > --- a/rust/kernel/platform.rs
> > +++ b/rust/kernel/platform.rs
> > @@ -64,6 +64,7 @@ extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> kernel::ff
> >          match T::probe(&mut pdev, info) {
> >              Ok(data) => {
> >                  let data = data.into_foreign();
> > +                let data = data.cast();
> >                  // Let the `struct platform_device` own a reference of the driver's private data.
> >                  // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
> >                  // `struct platform_device`.
> > @@ -78,6 +79,7 @@ extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> kernel::ff
> >      extern "C" fn remove_callback(pdev: *mut bindings::platform_device) {
> >          // SAFETY: `pdev` is a valid pointer to a `struct platform_device`.
> >          let ptr = unsafe { bindings::platform_get_drvdata(pdev) };
> > +        let ptr = ptr.cast();
> >
> >          // SAFETY: `remove_callback` is only ever called after a successful call to
> >          // `probe_callback`, hence it's guaranteed that `ptr` points to a valid and initialized
>

Re: [PATCH v16 2/4] rust: types: add `ForeignOwnable::PointedTo`

[Danilo Krummrich]

On Mon, Feb 17, 2025 at 09:02:12AM -0500, Tamir Duberstein wrote:
> > > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> > > index 6c3bc14b42ad..eb25fabbff9c 100644
> > > --- a/rust/kernel/pci.rs
> > > +++ b/rust/kernel/pci.rs
> > > @@ -73,6 +73,7 @@ extern "C" fn probe_callback(
> > >          match T::probe(&mut pdev, info) {
> > >              Ok(data) => {
> > >                  let data = data.into_foreign();
> > > +                let data = data.cast();
> >
> > Same here and below, see also [2].
> 
> You're the maintainer,

This isn't true. I'm the original author, but I'm not an official maintainer of
this code. :)

> so I'll do what you ask here as well. I did it
> this way because it avoids shadowing the git history with this change,
> which I thought was the dominant preference.

As mentioned in [2], if you do it the other way around first the "rust: types:
add `ForeignOwnable::PointedTo`" patch and then the conversion to cast() it's
even cleaner and less code to change.

> 
> > I understand you like this style and I'm not saying it's wrong or forbidden and
> > for code that you maintain such nits are entirely up to you as far as I'm
> > concerned.
> >
> > But I also don't think there is a necessity to convert things to your preference
> > wherever you touch existing code.
> 
> This isn't a conversion, it's a choice made specifically to avoid
> touching code that doesn't need to be touched (in this instance).

See above.

> 
> > I already explicitly asked you not to do so in [3] and yet you did so while
> > keeping my ACK. :(
> >
> > (Only saying the latter for reference, no need to send a new version of [3],
> > otherwise I would have replied.)
> >
> > [2] https://lore.kernel.org/rust-for-linux/Z7MYNQgo28sr_4RS@cassiopeiae/
> > [3] https://lore.kernel.org/rust-for-linux/20250213-aligned-alloc-v7-1-d2a2d0be164b@gmail.com/
> 
> I will drop [2] and leave the `as _` casts in place to minimize
> controversy here.

As mentioned I think the conversion to cast() is great, just do it after this
one and keep it a single line -- no controversy. :)

Re: [PATCH v16 2/4] rust: types: add `ForeignOwnable::PointedTo`

[Tamir Duberstein]

On Mon, Feb 17, 2025 at 9:15 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Mon, Feb 17, 2025 at 09:02:12AM -0500, Tamir Duberstein wrote:
> > > > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> > > > index 6c3bc14b42ad..eb25fabbff9c 100644
> > > > --- a/rust/kernel/pci.rs
> > > > +++ b/rust/kernel/pci.rs
> > > > @@ -73,6 +73,7 @@ extern "C" fn probe_callback(
> > > >          match T::probe(&mut pdev, info) {
> > > >              Ok(data) => {
> > > >                  let data = data.into_foreign();
> > > > +                let data = data.cast();
> > >
> > > Same here and below, see also [2].
> >
> > You're the maintainer,
>
> This isn't true. I'm the original author, but I'm not an official maintainer of
> this code. :)
>
> > so I'll do what you ask here as well. I did it
> > this way because it avoids shadowing the git history with this change,
> > which I thought was the dominant preference.
>
> As mentioned in [2], if you do it the other way around first the "rust: types:
> add `ForeignOwnable::PointedTo`" patch and then the conversion to cast() it's
> even cleaner and less code to change.

This is true for the two instances of `as _`, but not for all the
other instances where currently there's no cast, but one is now
needed.

> >
> > > I understand you like this style and I'm not saying it's wrong or forbidden and
> > > for code that you maintain such nits are entirely up to you as far as I'm
> > > concerned.
> > >
> > > But I also don't think there is a necessity to convert things to your preference
> > > wherever you touch existing code.
> >
> > This isn't a conversion, it's a choice made specifically to avoid
> > touching code that doesn't need to be touched (in this instance).
>
> See above.

This doesn't address my point. I claim that

@@ -246,6 +248,7 @@ impl<T: MiscDevice> VtableHelper<T> {
 ) -> c_int {
     // SAFETY: The release call of a file owns the private data.
     let private = unsafe { (*file).private_data };
+    let private = private.cast();
     // SAFETY: The release call of a file owns the private data.
     let ptr = unsafe { <T::Ptr as ForeignOwnable>::from_foreign(private) };

is a better diff than

@@ -245,7 +245,7 @@ impl<T: MiscDevice> VtableHelper<T> {
     file: *mut bindings::file,
 ) -> c_int {
     // SAFETY: The release call of a file owns the private data.
-    let private = unsafe { (*file).private_data };
+    let private = unsafe { (*file).private_data }.cast();
     // SAFETY: The release call of a file owns the private data.
     let ptr = unsafe { <T::Ptr as ForeignOwnable>::from_foreign(private) };

because it doesn't acquire the git blame on the existing line.

> >
> > > I already explicitly asked you not to do so in [3] and yet you did so while
> > > keeping my ACK. :(
> > >
> > > (Only saying the latter for reference, no need to send a new version of [3],
> > > otherwise I would have replied.)
> > >
> > > [2] https://lore.kernel.org/rust-for-linux/Z7MYNQgo28sr_4RS@cassiopeiae/
> > > [3] https://lore.kernel.org/rust-for-linux/20250213-aligned-alloc-v7-1-d2a2d0be164b@gmail.com/
> >
> > I will drop [2] and leave the `as _` casts in place to minimize
> > controversy here.
>
> As mentioned I think the conversion to cast() is great, just do it after this
> one and keep it a single line -- no controversy. :)

The code compiles either way, so I'll leave it untouched rather than
risk being scolded for sneaking unrelated changes.

Re: [PATCH v16 2/4] rust: types: add `ForeignOwnable::PointedTo`

[Danilo Krummrich]

On Mon, Feb 17, 2025 at 09:21:00AM -0500, Tamir Duberstein wrote:
> On Mon, Feb 17, 2025 at 9:15 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Mon, Feb 17, 2025 at 09:02:12AM -0500, Tamir Duberstein wrote:
> > > > > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> > > > > index 6c3bc14b42ad..eb25fabbff9c 100644
> > > > > --- a/rust/kernel/pci.rs
> > > > > +++ b/rust/kernel/pci.rs
> > > > > @@ -73,6 +73,7 @@ extern "C" fn probe_callback(
> > > > >          match T::probe(&mut pdev, info) {
> > > > >              Ok(data) => {
> > > > >                  let data = data.into_foreign();
> > > > > +                let data = data.cast();
> > > >
> > > > Same here and below, see also [2].
> > >
> > > You're the maintainer,
> >
> > This isn't true. I'm the original author, but I'm not an official maintainer of
> > this code. :)
> >
> > > so I'll do what you ask here as well. I did it
> > > this way because it avoids shadowing the git history with this change,
> > > which I thought was the dominant preference.
> >
> > As mentioned in [2], if you do it the other way around first the "rust: types:
> > add `ForeignOwnable::PointedTo`" patch and then the conversion to cast() it's
> > even cleaner and less code to change.
> 
> This is true for the two instances of `as _`,

Yes, those are the ones I talk about.

> but not for all the
> other instances where currently there's no cast, but one is now
> needed.
> 
> > >
> > > > I understand you like this style and I'm not saying it's wrong or forbidden and
> > > > for code that you maintain such nits are entirely up to you as far as I'm
> > > > concerned.
> > > >
> > > > But I also don't think there is a necessity to convert things to your preference
> > > > wherever you touch existing code.
> > >
> > > This isn't a conversion, it's a choice made specifically to avoid
> > > touching code that doesn't need to be touched (in this instance).
> >
> > See above.
> 
> This doesn't address my point. I claim that
> 
> @@ -246,6 +248,7 @@ impl<T: MiscDevice> VtableHelper<T> {
>  ) -> c_int {
>      // SAFETY: The release call of a file owns the private data.
>      let private = unsafe { (*file).private_data };
> +    let private = private.cast();
>      // SAFETY: The release call of a file owns the private data.
>      let ptr = unsafe { <T::Ptr as ForeignOwnable>::from_foreign(private) };
> 
> is a better diff than
> 
> @@ -245,7 +245,7 @@ impl<T: MiscDevice> VtableHelper<T> {
>      file: *mut bindings::file,
>  ) -> c_int {
>      // SAFETY: The release call of a file owns the private data.
> -    let private = unsafe { (*file).private_data };
> +    let private = unsafe { (*file).private_data }.cast();
>      // SAFETY: The release call of a file owns the private data.
>      let ptr = unsafe { <T::Ptr as ForeignOwnable>::from_foreign(private) };
> 
> because it doesn't acquire the git blame on the existing line.

I disagree with the *rationale*, because it would also mean that if I have

  let result = a + b;

and it turns out that we're off by one later on, it'd be reasonable to change it
to

  let result = a - b;
  let result = result + 1;

in order to not acquire the git blame of the existing line.

> 
> > >
> > > > I already explicitly asked you not to do so in [3] and yet you did so while
> > > > keeping my ACK. :(
> > > >
> > > > (Only saying the latter for reference, no need to send a new version of [3],
> > > > otherwise I would have replied.)
> > > >
> > > > [2] https://lore.kernel.org/rust-for-linux/Z7MYNQgo28sr_4RS@cassiopeiae/
> > > > [3] https://lore.kernel.org/rust-for-linux/20250213-aligned-alloc-v7-1-d2a2d0be164b@gmail.com/
> > >
> > > I will drop [2] and leave the `as _` casts in place to minimize
> > > controversy here.
> >
> > As mentioned I think the conversion to cast() is great, just do it after this
> > one and keep it a single line -- no controversy. :)
> 
> The code compiles either way, so I'll leave it untouched rather than
> risk being scolded for sneaking unrelated changes.

Again, I never did that, but as already mentioned if it came across this way,
please consider that I tell you now, that it wasn't meant to be.

You're free to do the change (I encourage that), but that's of course up to you.

Subsequently, I kindly ask you though to abstain from saying that I accused you
of something or do scold you. Thanks!

Re: [PATCH v16 2/4] rust: types: add `ForeignOwnable::PointedTo`

[Tamir Duberstein]

On Mon, Feb 17, 2025 at 9:37 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Mon, Feb 17, 2025 at 09:21:00AM -0500, Tamir Duberstein wrote:
> > On Mon, Feb 17, 2025 at 9:15 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > >
> > > On Mon, Feb 17, 2025 at 09:02:12AM -0500, Tamir Duberstein wrote:
> > > > > > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> > > > > > index 6c3bc14b42ad..eb25fabbff9c 100644
> > > > > > --- a/rust/kernel/pci.rs
> > > > > > +++ b/rust/kernel/pci.rs
> > > > > > @@ -73,6 +73,7 @@ extern "C" fn probe_callback(
> > > > > >          match T::probe(&mut pdev, info) {
> > > > > >              Ok(data) => {
> > > > > >                  let data = data.into_foreign();
> > > > > > +                let data = data.cast();
> > > > >
> > > > > Same here and below, see also [2].
> > > >
> > > > You're the maintainer,
> > >
> > > This isn't true. I'm the original author, but I'm not an official maintainer of
> > > this code. :)
> > >
> > > > so I'll do what you ask here as well. I did it
> > > > this way because it avoids shadowing the git history with this change,
> > > > which I thought was the dominant preference.
> > >
> > > As mentioned in [2], if you do it the other way around first the "rust: types:
> > > add `ForeignOwnable::PointedTo`" patch and then the conversion to cast() it's
> > > even cleaner and less code to change.
> >
> > This is true for the two instances of `as _`,
>
> Yes, those are the ones I talk about.
>
> > but not for all the
> > other instances where currently there's no cast, but one is now
> > needed.
> >
> > > >
> > > > > I understand you like this style and I'm not saying it's wrong or forbidden and
> > > > > for code that you maintain such nits are entirely up to you as far as I'm
> > > > > concerned.
> > > > >
> > > > > But I also don't think there is a necessity to convert things to your preference
> > > > > wherever you touch existing code.
> > > >
> > > > This isn't a conversion, it's a choice made specifically to avoid
> > > > touching code that doesn't need to be touched (in this instance).
> > >
> > > See above.
> >
> > This doesn't address my point. I claim that
> >
> > @@ -246,6 +248,7 @@ impl<T: MiscDevice> VtableHelper<T> {
> >  ) -> c_int {
> >      // SAFETY: The release call of a file owns the private data.
> >      let private = unsafe { (*file).private_data };
> > +    let private = private.cast();
> >      // SAFETY: The release call of a file owns the private data.
> >      let ptr = unsafe { <T::Ptr as ForeignOwnable>::from_foreign(private) };
> >
> > is a better diff than
> >
> > @@ -245,7 +245,7 @@ impl<T: MiscDevice> VtableHelper<T> {
> >      file: *mut bindings::file,
> >  ) -> c_int {
> >      // SAFETY: The release call of a file owns the private data.
> > -    let private = unsafe { (*file).private_data };
> > +    let private = unsafe { (*file).private_data }.cast();
> >      // SAFETY: The release call of a file owns the private data.
> >      let ptr = unsafe { <T::Ptr as ForeignOwnable>::from_foreign(private) };
> >
> > because it doesn't acquire the git blame on the existing line.
>
> I disagree with the *rationale*, because it would also mean that if I have
>
>   let result = a + b;
>
> and it turns out that we're off by one later on, it'd be reasonable to change it
> to
>
>   let result = a - b;
>   let result = result + 1;
>
> in order to not acquire the git blame of the existing line.

Like anything, it depends. If something changes from being 0-indexed
to 1-indexed then I'd say what you have there is perfectly reasonable:
the 1-bias is logically separate from `a - b`. That's a fine analogy
for what's happening in this patch.

> >
> > > >
> > > > > I already explicitly asked you not to do so in [3] and yet you did so while
> > > > > keeping my ACK. :(
> > > > >
> > > > > (Only saying the latter for reference, no need to send a new version of [3],
> > > > > otherwise I would have replied.)
> > > > >
> > > > > [2] https://lore.kernel.org/rust-for-linux/Z7MYNQgo28sr_4RS@cassiopeiae/
> > > > > [3] https://lore.kernel.org/rust-for-linux/20250213-aligned-alloc-v7-1-d2a2d0be164b@gmail.com/
> > > >
> > > > I will drop [2] and leave the `as _` casts in place to minimize
> > > > controversy here.
> > >
> > > As mentioned I think the conversion to cast() is great, just do it after this
> > > one and keep it a single line -- no controversy. :)
> >
> > The code compiles either way, so I'll leave it untouched rather than
> > risk being scolded for sneaking unrelated changes.
>
> Again, I never did that, but as already mentioned if it came across this way,
> please consider that I tell you now, that it wasn't meant to be.

Wasn't my intention to imply that this was something you did. It was
meant as a general observation.

> You're free to do the change (I encourage that), but that's of course up to you.

I'll create a "good first issue" for it in the RfL repository.

> Subsequently, I kindly ask you though to abstain from saying that I accused you
> of something or do scold you. Thanks!

Certainly. I'll point out, as you did, that I never said that.

Re: [PATCH v16 2/4] rust: types: add `ForeignOwnable::PointedTo`

[Danilo Krummrich]

On Mon, Feb 17, 2025 at 09:47:14AM -0500, Tamir Duberstein wrote:
> On Mon, Feb 17, 2025 at 9:37 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > You're free to do the change (I encourage that), but that's of course up to you.
> 
> I'll create a "good first issue" for it in the RfL repository.

That's a good idea -- thanks.

Re: [PATCH v16 2/4] rust: types: add `ForeignOwnable::PointedTo`

[Tamir Duberstein]

On Mon, Feb 17, 2025 at 9:52 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Mon, Feb 17, 2025 at 09:47:14AM -0500, Tamir Duberstein wrote:
> > On Mon, Feb 17, 2025 at 9:37 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > > You're free to do the change (I encourage that), but that's of course up to you.
> >
> > I'll create a "good first issue" for it in the RfL repository.
>
> That's a good idea -- thanks.

What do you think about enabling clippy::ptr_as_ptr?
https://rust-lang.github.io/rust-clippy/master/index.html#ptr_as_ptr

Do we currently enable any non-default clippy lints?

Re: [PATCH v16 2/4] rust: types: add `ForeignOwnable::PointedTo`

[Danilo Krummrich]

On Mon, Feb 17, 2025 at 10:50:10AM -0500, Tamir Duberstein wrote:
> On Mon, Feb 17, 2025 at 9:52 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Mon, Feb 17, 2025 at 09:47:14AM -0500, Tamir Duberstein wrote:
> > > On Mon, Feb 17, 2025 at 9:37 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > > > You're free to do the change (I encourage that), but that's of course up to you.
> > >
> > > I'll create a "good first issue" for it in the RfL repository.
> >
> > That's a good idea -- thanks.
> 
> What do you think about enabling clippy::ptr_as_ptr?
> https://rust-lang.github.io/rust-clippy/master/index.html#ptr_as_ptr

Seems reasonable to me, but I'm a bit out of competence here.

I know some lints are not stable and hence need to be treated with care, though
this doesn't seem to be one of them.

Additionally, I think the lint would need to be supported by all compiler
versions the kernel supports currently, which also seems to be the case (added
in: 1.51.0).

> 
> Do we currently enable any non-default clippy lints?

Yes, you can check the root Makefile [1].

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Makefile?h=v6.14-rc3#n471

Re: [PATCH v16 2/4] rust: types: add `ForeignOwnable::PointedTo`

[Miguel Ojeda]

On Mon, Feb 17, 2025 at 3:21 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> because it doesn't acquire the git blame on the existing line.

Hmm... What did give you the impression that we need to avoid that?

Cheers,
Miguel

Re: [PATCH v16 2/4] rust: types: add `ForeignOwnable::PointedTo`

[Miguel Ojeda]

On Mon, Feb 17, 2025 at 4:50 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> What do you think about enabling clippy::ptr_as_ptr?
> https://rust-lang.github.io/rust-clippy/master/index.html#ptr_as_ptr

`ptr_as_ptr` is one of the ones Trevor suggested back then.

It seems good to me, but there are quite a lot of instances already
around, so some cleaning would be needed.

Cheers,
Miguel

Re: [PATCH v16 2/4] rust: types: add `ForeignOwnable::PointedTo`

[Miguel Ojeda]

On Mon, Feb 17, 2025 at 5:35 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> I know some lints are not stable and hence need to be treated with care, though
> this doesn't seem to be one of them.

Yeah, the "nursery" ones may be not ready for prime time.

> Additionally, I think the lint would need to be supported by all compiler
> versions the kernel supports currently, which also seems to be the case (added
> in: 1.51.0).

Yeah, though we could ignore unknown ones.

Cheers,
Miguel

Re: [PATCH v16 2/4] rust: types: add `ForeignOwnable::PointedTo`

[Tamir Duberstein]

On Mon, Feb 17, 2025 at 12:03 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Mon, Feb 17, 2025 at 3:21 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > because it doesn't acquire the git blame on the existing line.
>
> Hmm... What did give you the impression that we need to avoid that?

Only my personal experience with git blame. The `.cast()` call itself
is boilerplate that arises from the difference in types between the
abstractions and the bindings; my opinion is that a user of git blame
is more likely to be interested in the rationale accompanying the
implementation rather than that of the type shuffle.

Tamir

Re: [PATCH v16 2/4] rust: types: add `ForeignOwnable::PointedTo`

[Miguel Ojeda]

On Mon, Feb 17, 2025 at 6:11 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> Only my personal experience with git blame. The `.cast()` call itself
> is boilerplate that arises from the difference in types between the
> abstractions and the bindings; my opinion is that a user of git blame
> is more likely to be interested in the rationale accompanying the
> implementation rather than that of the type shuffle.

I understand the rationale -- what I meant to ask is if you saw that
rule in kernel documentation or similar, because I could be missing
something, but I don't think we do that in the kernel.

So if you have a change like that, please just change the line, rather
than adding new ones just for `git blame`.

Cheers,
Miguel

Re: [PATCH v16 2/4] rust: types: add `ForeignOwnable::PointedTo`

[Miguel Ojeda]

On Mon, Feb 17, 2025 at 6:24 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> I understand the rationale -- what I meant to ask is if you saw that

By the way, I don't agree with the rationale, because it sounds to me
like optimizing for `git blame` readers, while pessimizing for normal
readers.

We do a lot of `git blame` in the kernel, especially since our Git log
is quite good, but we still read the files themselves more... I can
imagine ending up with a lot of extra lines over time everywhere, it
could dissuade small fixes and so on.

Cheers,
Miguel

Re: [PATCH v16 2/4] rust: types: add `ForeignOwnable::PointedTo`

[Tamir Duberstein]

On Mon, Feb 17, 2025 at 12:36 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Mon, Feb 17, 2025 at 6:24 PM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
> >
> > I understand the rationale -- what I meant to ask is if you saw that
>
> By the way, I don't agree with the rationale, because it sounds to me
> like optimizing for `git blame` readers, while pessimizing for normal
> readers.
>
> We do a lot of `git blame` in the kernel, especially since our Git log
> is quite good, but we still read the files themselves more... I can
> imagine ending up with a lot of extra lines over time everywhere, it
> could dissuade small fixes and so on.

I almost addressed this in my original reply - I regret that I didn't.

I agree with you that optimizing for git blame while pessimizing for
normal readers is not what we should do. I don't agree that putting
boilerplate on its own line is a pessimization for the normal reader -
in my opinion it is the opposite. Trivial expressions of the form

let foo = foo.cast();

can be very easily skimmed by a reader, whereas an expression of the form

unsafe { <type as trait>::associated_type::function(foo.cast()) }

become more difficult to read with every operation that is added to it.

As always, this is just my opinion.

Re: [PATCH v16 2/4] rust: types: add `ForeignOwnable::PointedTo`

[Miguel Ojeda]

On Mon, Feb 17, 2025 at 6:44 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> I agree with you that optimizing for git blame while pessimizing for
> normal readers is not what we should do. I don't agree that putting
> boilerplate on its own line is a pessimization for the normal reader -
> in my opinion it is the opposite. Trivial expressions of the form

But that is a different argument, unrelated to `git blame`, no?

What I was saying is that, if the only reason one is adding a line is
for `git blame`, then it shouldn't be done.

But, of course, if there is a different, good reason to add a line,
then it should be done.

In other words, `git blame` does not play a role here.

I mean, a reasonable person could say it should at least have a small
weight into the decision, sure. But I don't think we currently do that
and it makes decisions even more complex...

Cheers,
Miguel

Re: [PATCH v16 2/4] rust: types: add `ForeignOwnable::PointedTo`

[Tamir Duberstein]

On Mon, Feb 17, 2025 at 1:13 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Mon, Feb 17, 2025 at 6:44 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > I agree with you that optimizing for git blame while pessimizing for
> > normal readers is not what we should do. I don't agree that putting
> > boilerplate on its own line is a pessimization for the normal reader -
> > in my opinion it is the opposite. Trivial expressions of the form
>
> But that is a different argument, unrelated to `git blame`, no?

Yes it is. I suppose I misunderstood your objection to this rationale,
along with

> So if you have a change like that, please just change the line, rather
than adding new ones just for `git blame`.

as an objection to the decision, so I was giving additional rationale.

> What I was saying is that, if the only reason one is adding a line is
> for `git blame`, then it shouldn't be done.
>
> But, of course, if there is a different, good reason to add a line,
> then it should be done.
>
> In other words, `git blame` does not play a role here.
>
> I mean, a reasonable person could say it should at least have a small
> weight into the decision, sure. But I don't think we currently do that
> and it makes decisions even more complex...

Unclear where this leaves us so I'll just go with .cast() in-line.

Re: [PATCH v16 2/4] rust: types: add `ForeignOwnable::PointedTo`

[Andreas Hindborg]

"Danilo Krummrich" <dakr@kernel.org> writes:

> On Fri, Feb 07, 2025 at 08:58:25AM -0500, Tamir Duberstein wrote:
>> Allow implementors to specify the foreign pointer type; this exposes
>> information about the pointed-to type such as its alignment.
>>
>> This requires the trait to be `unsafe` since it is now possible for
>> implementors to break soundness by returning a misaligned pointer.
>>
>> Encoding the pointer type in the trait (and avoiding pointer casts)
>> allows the compiler to check that implementors return the correct
>> pointer type. This is preferable to directly encoding the alignment in
>> the trait using a constant as the compiler would be unable to check it.
>>
>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
>> Reviewed-by: Fiona Behrens <me@kloenk.dev>
>
> I know that Andreas also asked you to pick up the RBs from [1], but - without
> speaking for any of the people above - given that you changed this commit after
> you received all those RBs you should also consider dropping them. Especially,
> since you do not mention the changes you did for this commit in the version
> history.
>
> Just to be clear, often it is also fine to keep tags for minor changes, but then
> you should make people aware of them in the version history, such that they get
> the chance to double check.
>
> [1] https://lore.kernel.org/rust-for-linux/20250131-configfs-v1-1-87947611401c@kernel.org/
>

As long as the commit was not radically changed, I see no point in
dropping the commit trailers. Same policy as for dropping review tags
when issuing a new version of a series should be applied.


Best regards,
Andreas Hindborg