[PATCH v4 0/3] rust: miscdevice: add additional data to MiscDeviceRegistration

[PATCH v4 0/3] rust: miscdevice: add additional data to MiscDeviceRegistration

[Christian Schrefl]

Currently there is no good way to pass arbitrary data from the driver to
a `miscdevice` or to share data between individual handles to a 
`miscdevice` in rust.

This series adds additional (generic) data to the MiscDeviceRegistration
for this purpose.

The first patch originally comes from my `UnsafePinned` patch series [0].

The second patch implements the changes and fixes the build of the sample
without changing any functionality (this is currently the only in tree 
user).

The third patch changes the `rust_misc_device` sample to use this to 
share the same data between multiple handles to the `miscdevice`.
I have tested the sample with qemu and the C userspace example
from the doc comments.

Some discussion on Zulip about the motivation and approach [1].
Thanks a lot to everyone helping me out with this.

This patch series is based on the rust-next branch.

Link: https://lore.kernel.org/rust-for-linux/20250511-rust_unsafe_pinned-v4-2-a86c32e47e3d@gmail.com/  [0]
Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Passing.20a.20DevRes.20to.20a.20miscdev/near/494553814 [1]

Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
---
Changes in v4:
- Rework to use Opaque instead of `UnsafePinned`.
- Include `impl Wrapper for Opaque` patch.
- Link to v3: https://lore.kernel.org/r/20250517-b4-rust_miscdevice_registrationdata-v3-0-cdb33e228d37@gmail.com

Changes in v3:
- Rebased on top of my `UnsafePinned` series.
- Link to v2: https://lore.kernel.org/r/20250131-b4-rust_miscdevice_registrationdata-v2-0-588f1e6cfabe@gmail.com

Changes in v2:
- Don't use associated_type_bounds since the MSRV does not support
    that on stable yet (Kernel test robot)
- Doc changes and add intra-doc links (Miguel)
- Use container_of macro instead of pointer cast in `fops_open` (Greg)
- Rename `Aliased` to `UnsafePinned` (Boqun)
- Make sure Data is initialized before `misc_register` is called
- Rework the example to use an additional shared value instead of 
    replacing the unique one
- Expanded the c code for the example to use the new ioctls
- Link to v1: https://lore.kernel.org/r/20250119-b4-rust_miscdevice_registrationdata-v1-0-edbf18dde5fc@gmail.com

---
Christian Schrefl (3):
      rust: implement `Wrapper<T>` for `Opaque<T>`
      rust: miscdevice: add additional data to MiscDeviceRegistration
      rust: miscdevice: adjust the rust_misc_device sample to use RegistrationData.

 rust/kernel/miscdevice.rs        |  79 +++++++++++++++++++-------
 rust/kernel/revocable.rs         |   2 +
 rust/kernel/types.rs             |  25 ++++----
 samples/rust/rust_misc_device.rs | 120 ++++++++++++++++++++++++++++++++++++---
 4 files changed, 185 insertions(+), 41 deletions(-)
---
base-commit: a3b2347343e077e81d3c169f32c9b2cb1364f4cc
change-id: 20250119-b4-rust_miscdevice_registrationdata-a11d88dcb284

Best regards,
-- 
Christian Schrefl <chrisi.schrefl@gmail.com>

[PATCH v4 1/3] rust: implement `Wrapper<T>` for `Opaque<T>`

[Christian Schrefl]

Moves the implementation for `pin-init` from an associated function
to the trait function of the `Wrapper` trait and extends the
implementation to support pin-initializers with error types.

Adds a use for the `Wrapper` trait in `revocable.rs`, to use the new
`pin-init` function. This is currently the only usage in the kernel.

Reviewed-by: Gerald Wisböck <gerald.wisboeck@feather.ink>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Benno Lossin <lossin@kernel.org>
Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
---
 rust/kernel/revocable.rs |  2 ++
 rust/kernel/types.rs     | 25 +++++++++++++------------
 2 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
index 1e5a9d25c21b279b01f90b02997492aa4880d84f..4db68ea2207ebafcc09d082fdc1e281f31846a38 100644
--- a/rust/kernel/revocable.rs
+++ b/rust/kernel/revocable.rs
@@ -5,6 +5,8 @@
 //! The [`Revocable`] type wraps other types and allows access to them to be revoked. The existence
 //! of a [`RevocableGuard`] ensures that objects remain valid.
 
+use pin_init::Wrapper;
+
 use crate::{bindings, prelude::*, sync::rcu, types::Opaque};
 use core::{
     marker::PhantomData,
diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 86562e738eac85480a048041e979335b81c5e3c9..7ab70d5f76099c3442dce5b02c6b226fc74c851e 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -9,7 +9,7 @@
     ops::{Deref, DerefMut},
     ptr::NonNull,
 };
-use pin_init::{PinInit, Zeroable};
+use pin_init::{PinInit, Wrapper, Zeroable};
 
 /// Used to transfer ownership to and from foreign (non-Rust) languages.
 ///
@@ -345,17 +345,6 @@ pub const fn uninit() -> Self {
         }
     }
 
-    /// Create an opaque pin-initializer from the given pin-initializer.
-    pub fn pin_init(slot: impl PinInit<T>) -> impl PinInit<Self> {
-        Self::ffi_init(|ptr: *mut T| {
-            // SAFETY:
-            //   - `ptr` is a valid pointer to uninitialized memory,
-            //   - `slot` is not accessed on error; the call is infallible,
-            //   - `slot` is pinned in memory.
-            let _ = unsafe { PinInit::<T>::__pinned_init(slot, ptr) };
-        })
-    }
-
     /// Creates a pin-initializer from the given initializer closure.
     ///
     /// The returned initializer calls the given closure with the pointer to the inner `T` of this
@@ -406,6 +395,18 @@ pub const fn raw_get(this: *const Self) -> *mut T {
         UnsafeCell::raw_get(this.cast::<UnsafeCell<MaybeUninit<T>>>()).cast::<T>()
     }
 }
+impl<T> Wrapper<T> for Opaque<T> {
+    /// Create an opaque pin-initializer from the given pin-initializer.
+    fn pin_init<E>(slot: impl PinInit<T, E>) -> impl PinInit<Self, E> {
+        Self::try_ffi_init(|ptr: *mut T| {
+            // SAFETY:
+            //   - `ptr` is a valid pointer to uninitialized memory,
+            //   - `slot` is not accessed on error,
+            //   - `slot` is pinned in memory.
+            unsafe { PinInit::<T, E>::__pinned_init(slot, ptr) }
+        })
+    }
+}
 
 /// Types that are _always_ reference counted.
 ///

-- 
2.49.0

Re: [PATCH v4 1/3] rust: implement `Wrapper<T>` for `Opaque<T>`

[Christian Schrefl]

On 30.05.25 10:46 PM, Christian Schrefl wrote:
> Moves the implementation for `pin-init` from an associated function
> to the trait function of the `Wrapper` trait and extends the
> implementation to support pin-initializers with error types.
> 
> Adds a use for the `Wrapper` trait in `revocable.rs`, to use the new
> `pin-init` function. This is currently the only usage in the kernel.
> 
> Reviewed-by: Gerald Wisböck <gerald.wisboeck@feather.ink>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Benno Lossin <lossin@kernel.org>
> Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
> ---

Danilo, FYI this is basically a patch from my `UnsafePinned`
series [0] that I've used instead of your patch [1] that does something similar.
(I've only dropped `the call is infallible` from the safety
comment like in your patch since I missed that before).

Let me know if you want me to handle this any different.

[0]: https://lore.kernel.org/rust-for-linux/20250511-rust_unsafe_pinned-v4-2-a86c32e47e3d@gmail.com/
[1]: https://lore.kernel.org/rust-for-linux/20250530142447.166524-2-dakr@kernel.org/

Cheers
Christian

>  rust/kernel/revocable.rs |  2 ++
>  rust/kernel/types.rs     | 25 +++++++++++++------------
>  2 files changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> index 1e5a9d25c21b279b01f90b02997492aa4880d84f..4db68ea2207ebafcc09d082fdc1e281f31846a38 100644
> --- a/rust/kernel/revocable.rs
> +++ b/rust/kernel/revocable.rs
> @@ -5,6 +5,8 @@
>  //! The [`Revocable`] type wraps other types and allows access to them to be revoked. The existence
>  //! of a [`RevocableGuard`] ensures that objects remain valid.
>  
> +use pin_init::Wrapper;
> +
>  use crate::{bindings, prelude::*, sync::rcu, types::Opaque};
>  use core::{
>      marker::PhantomData,
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 86562e738eac85480a048041e979335b81c5e3c9..7ab70d5f76099c3442dce5b02c6b226fc74c851e 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -9,7 +9,7 @@
>      ops::{Deref, DerefMut},
>      ptr::NonNull,
>  };
> -use pin_init::{PinInit, Zeroable};
> +use pin_init::{PinInit, Wrapper, Zeroable};
>  
>  /// Used to transfer ownership to and from foreign (non-Rust) languages.
>  ///
> @@ -345,17 +345,6 @@ pub const fn uninit() -> Self {
>          }
>      }
>  
> -    /// Create an opaque pin-initializer from the given pin-initializer.
> -    pub fn pin_init(slot: impl PinInit<T>) -> impl PinInit<Self> {
> -        Self::ffi_init(|ptr: *mut T| {
> -            // SAFETY:
> -            //   - `ptr` is a valid pointer to uninitialized memory,
> -            //   - `slot` is not accessed on error; the call is infallible,
> -            //   - `slot` is pinned in memory.
> -            let _ = unsafe { PinInit::<T>::__pinned_init(slot, ptr) };
> -        })
> -    }
> -
>      /// Creates a pin-initializer from the given initializer closure.
>      ///
>      /// The returned initializer calls the given closure with the pointer to the inner `T` of this
> @@ -406,6 +395,18 @@ pub const fn raw_get(this: *const Self) -> *mut T {
>          UnsafeCell::raw_get(this.cast::<UnsafeCell<MaybeUninit<T>>>()).cast::<T>()
>      }
>  }
> +impl<T> Wrapper<T> for Opaque<T> {
> +    /// Create an opaque pin-initializer from the given pin-initializer.
> +    fn pin_init<E>(slot: impl PinInit<T, E>) -> impl PinInit<Self, E> {
> +        Self::try_ffi_init(|ptr: *mut T| {
> +            // SAFETY:
> +            //   - `ptr` is a valid pointer to uninitialized memory,
> +            //   - `slot` is not accessed on error,
> +            //   - `slot` is pinned in memory.
> +            unsafe { PinInit::<T, E>::__pinned_init(slot, ptr) }
> +        })
> +    }
> +}
>  
>  /// Types that are _always_ reference counted.
>  ///
> 

Re: [PATCH v4 1/3] rust: implement `Wrapper<T>` for `Opaque<T>`

[Danilo Krummrich]

On Fri, May 30, 2025 at 10:53:21PM +0200, Christian Schrefl wrote:
> On 30.05.25 10:46 PM, Christian Schrefl wrote:
> > Moves the implementation for `pin-init` from an associated function
> > to the trait function of the `Wrapper` trait and extends the
> > implementation to support pin-initializers with error types.
> > 
> > Adds a use for the `Wrapper` trait in `revocable.rs`, to use the new
> > `pin-init` function. This is currently the only usage in the kernel.
> > 
> > Reviewed-by: Gerald Wisböck <gerald.wisboeck@feather.ink>
> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > Reviewed-by: Benno Lossin <lossin@kernel.org>
> > Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
> > ---
> 
> Danilo, FYI this is basically a patch from my `UnsafePinned`
> series [0] that I've used instead of your patch [1] that does something similar.
> (I've only dropped `the call is infallible` from the safety
> comment like in your patch since I missed that before).
> 
> Let me know if you want me to handle this any different.

No, that's fine, I wasn't aware of this patch, let's go with this one then.

	Reviewed-by: Danilo Krummrich <dakr@kernel.org>

> [0]: https://lore.kernel.org/rust-for-linux/20250511-rust_unsafe_pinned-v4-2-a86c32e47e3d@gmail.com/
> [1]: https://lore.kernel.org/rust-for-linux/20250530142447.166524-2-dakr@kernel.org/

[PATCH v4 2/3] rust: miscdevice: add additional data to MiscDeviceRegistration

[Christian Schrefl]

When using the Rust miscdevice bindings, you generally embed the
`MiscDeviceRegistration` within another struct:

struct MyDriverData {
    data: SomeOtherData,
    misc: MiscDeviceRegistration<MyMiscFile>
}

In the `fops->open` callback of the miscdevice, you are given a
reference to the registration, which allows you to access its fields.
For example, as of commit 284ae0be4dca ("rust: miscdevice: Provide
accessor to pull out miscdevice::this_device") you can access the
internal `struct device`. However, there is still no way to access the
`data` field in the above example, because you only have a reference to
the registration.

Using `container_of` is also not possible to do safely. For example, if
the destructor of `MyDriverData` runs, then the destructor of `data`
would run before the miscdevice is deregistered, so using `container_of`
to access `data` from `fops->open` could result in a UAF. A similar
problem can happen on initialization if `misc` is not the last field to
be initialized.

To provide a safe way to access user-defined data stored next to the
`struct miscdevice`, make `MiscDeviceRegistration` into a container that
can store a user-provided piece of data. This way, `fops->open` can
access that data via the registration, since the data is stored inside
the registration.

The container enforces that the additional user data is initialized
before the miscdevice is registered, and that the miscdevice is
deregistered before the user data is destroyed. This ensures that access
to the userdata is safe.

For the same reasons as in commit 88441d5c6d17 ("rust: miscdevice:
access the `struct miscdevice` from fops->open()"), you cannot access
the user data in any other fops callback than open. This is because a
miscdevice can be deregistered while there are still open files.

A situation where this user data might be required is when a platform
driver acquires a resource in `probe` and wants to use this resource in
the `fops` implementation of a `MiscDevice`.

This solution is similar to the approach used by the initial downstream
Rust-for-Linux/Rust branch [0].

Link: https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/miscdev.rs#L108 [0]
Suggested-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
---
 rust/kernel/miscdevice.rs        | 79 ++++++++++++++++++++++++++++++----------
 samples/rust/rust_misc_device.rs |  4 +-
 2 files changed, 62 insertions(+), 21 deletions(-)

diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
index b4c5f74de23d6f4fbcdebfe408d6954884609e8f..ad9fc0b2383860cb976c57a398f372280c19513c 100644
--- a/rust/kernel/miscdevice.rs
+++ b/rust/kernel/miscdevice.rs
@@ -9,7 +9,7 @@
 //! Reference: <https://www.kernel.org/doc/html/latest/driver-api/misc_devices.html>
 
 use crate::{
-    bindings,
+    bindings, container_of,
     device::Device,
     error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR},
     ffi::{c_int, c_long, c_uint, c_ulong},
@@ -20,6 +20,7 @@
     types::{ForeignOwnable, Opaque},
 };
 use core::{marker::PhantomData, mem::MaybeUninit, pin::Pin};
+use pin_init::Wrapper;
 
 /// Options for creating a misc device.
 #[derive(Copy, Clone)]
@@ -45,32 +46,46 @@ pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
 /// # Invariants
 ///
 /// `inner` is a registered misc device.
-#[repr(transparent)]
+#[repr(C)]
 #[pin_data(PinnedDrop)]
-pub struct MiscDeviceRegistration<T> {
+pub struct MiscDeviceRegistration<T: MiscDevice> {
     #[pin]
     inner: Opaque<bindings::miscdevice>,
+    #[pin]
+    data: Opaque<T::RegistrationData>,
     _t: PhantomData<T>,
 }
 
-// SAFETY: It is allowed to call `misc_deregister` on a different thread from where you called
-// `misc_register`.
-unsafe impl<T> Send for MiscDeviceRegistration<T> {}
-// SAFETY: All `&self` methods on this type are written to ensure that it is safe to call them in
-// parallel.
-unsafe impl<T> Sync for MiscDeviceRegistration<T> {}
+// SAFETY:
+// - It is allowed to call `misc_deregister` on a different thread from where you called
+//   `misc_register`.
+// - Only implements `Send` if `MiscDevice::RegistrationData` is also `Send`.
+unsafe impl<T: MiscDevice> Send for MiscDeviceRegistration<T> where T::RegistrationData: Send {}
+
+// SAFETY:
+// - All `&self` methods on this type are written to ensure that it is safe to call them in
+//   parallel.
+// - `MiscDevice::RegistrationData` is always `Sync`.
+unsafe impl<T: MiscDevice> Sync for MiscDeviceRegistration<T> {}
 
 impl<T: MiscDevice> MiscDeviceRegistration<T> {
     /// Register a misc device.
-    pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
+    pub fn register(
+        opts: MiscDeviceOptions,
+        data: impl PinInit<T::RegistrationData, Error>,
+    ) -> impl PinInit<Self, Error> {
         try_pin_init!(Self {
+            data <- Opaque::pin_init(data),
             inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
                 // SAFETY: The initializer can write to the provided `slot`.
                 unsafe { slot.write(opts.into_raw::<T>()) };
 
-                // SAFETY: We just wrote the misc device options to the slot. The miscdevice will
-                // get unregistered before `slot` is deallocated because the memory is pinned and
-                // the destructor of this type deallocates the memory.
+                // SAFETY:
+                // * We just wrote the misc device options to the slot. The miscdevice will
+                //   get unregistered before `slot` is deallocated because the memory is pinned and
+                //   the destructor of this type deallocates the memory.
+                // * `data` is Initialized before `misc_register` so no race with `fops->open()`
+                //   is possible.
                 // INVARIANT: If this returns `Ok(())`, then the `slot` will contain a registered
                 // misc device.
                 to_result(unsafe { bindings::misc_register(slot) })
@@ -93,13 +108,24 @@ pub fn device(&self) -> &Device {
         // before the underlying `struct miscdevice` is destroyed.
         unsafe { Device::as_ref((*self.as_raw()).this_device) }
     }
+
+    /// Access the additional data stored in this registration.
+    pub fn data(&self) -> &T::RegistrationData {
+        // SAFETY:
+        // * No mutable reference to the value contained by `self.data` can ever be created.
+        // * The value contained by `self.data` is valid for the entire lifetime of `&self`.
+        unsafe { &*self.data.get() }
+    }
 }
 
 #[pinned_drop]
-impl<T> PinnedDrop for MiscDeviceRegistration<T> {
+impl<T: MiscDevice> PinnedDrop for MiscDeviceRegistration<T> {
     fn drop(self: Pin<&mut Self>) {
         // SAFETY: We know that the device is registered by the type invariants.
         unsafe { bindings::misc_deregister(self.inner.get()) };
+
+        // SAFETY: `self.data` is valid for dropping and nothing uses it anymore.
+        unsafe { core::ptr::drop_in_place(self.data.get()) };
     }
 }
 
@@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
     /// What kind of pointer should `Self` be wrapped in.
     type Ptr: ForeignOwnable + Send + Sync;
 
+    /// The additional data carried by the [`MiscDeviceRegistration`] for this [`MiscDevice`].
+    /// If no additional data is required than the unit type `()` should be used.
+    ///
+    /// This data can be accessed in [`MiscDevice::open()`] using
+    /// [`MiscDeviceRegistration::data()`].
+    type RegistrationData: Sync;
+
     /// Called when the misc device is opened.
     ///
     /// The returned pointer will be stored as the private data for the file.
@@ -178,18 +211,24 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
         // SAFETY: The open call of a file can access the private data.
         let misc_ptr = unsafe { (*raw_file).private_data };
 
-        // SAFETY: This is a miscdevice, so `misc_open()` set the private data to a pointer to the
-        // associated `struct miscdevice` before calling into this method. Furthermore,
-        // `misc_open()` ensures that the miscdevice can't be unregistered and freed during this
-        // call to `fops_open`.
-        let misc = unsafe { &*misc_ptr.cast::<MiscDeviceRegistration<T>>() };
+        // This is a miscdevice, so `misc_open()` sets the private data to a pointer to the
+        // associated `struct miscdevice` before calling into this method.
+        let misc_ptr = misc_ptr.cast::<bindings::miscdevice>();
+
+        // SAFETY:
+        // * `misc_open()` ensures that the `struct miscdevice` can't be unregistered and freed
+        //   during this call to `fops_open`.
+        // * The `misc_ptr` always points to the `inner` field of a `MiscDeviceRegistration<T>`.
+        // * The `MiscDeviceRegistration<T>` is valid until the `struct miscdevice` was
+        //   unregistered.
+        let registration = unsafe { &*container_of!(misc_ptr, MiscDeviceRegistration<T>, inner) };
 
         // SAFETY:
         // * This underlying file is valid for (much longer than) the duration of `T::open`.
         // * There is no active fdget_pos region on the file on this thread.
         let file = unsafe { File::from_raw_file(raw_file) };
 
-        let ptr = match T::open(file, misc) {
+        let ptr = match T::open(file, registration) {
             Ok(ptr) => ptr,
             Err(err) => return err.to_errno(),
         };
diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
index c881fd6dbd08cf4308fe1bd37d11d28374c1f034..67a6172fbbf72dd42a1b655f5f5a782101432707 100644
--- a/samples/rust/rust_misc_device.rs
+++ b/samples/rust/rust_misc_device.rs
@@ -137,7 +137,7 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
         };
 
         try_pin_init!(Self {
-            _miscdev <- MiscDeviceRegistration::register(options),
+            _miscdev <- MiscDeviceRegistration::register(options, ()),
         })
     }
 }
@@ -157,6 +157,8 @@ struct RustMiscDevice {
 impl MiscDevice for RustMiscDevice {
     type Ptr = Pin<KBox<Self>>;
 
+    type RegistrationData = ();
+
     fn open(_file: &File, misc: &MiscDeviceRegistration<Self>) -> Result<Pin<KBox<Self>>> {
         let dev = ARef::from(misc.device());
 

-- 
2.49.0

Re: [PATCH v4 2/3] rust: miscdevice: add additional data to MiscDeviceRegistration

[Benno Lossin]

On Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
> @@ -45,32 +46,46 @@ pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
>  /// # Invariants
>  ///
>  /// `inner` is a registered misc device.
> -#[repr(transparent)]
> +#[repr(C)]

Why do we need linear layout? `container_of!` also works with the `Rust`
layout.

>  #[pin_data(PinnedDrop)]
> -pub struct MiscDeviceRegistration<T> {
> +pub struct MiscDeviceRegistration<T: MiscDevice> {
>      #[pin]
>      inner: Opaque<bindings::miscdevice>,
> +    #[pin]
> +    data: Opaque<T::RegistrationData>,
>      _t: PhantomData<T>,

No need to keep the `PhantomData` field around, since you're using `T`
above.

>  }
>  
> -// SAFETY: It is allowed to call `misc_deregister` on a different thread from where you called
> -// `misc_register`.
> -unsafe impl<T> Send for MiscDeviceRegistration<T> {}
> -// SAFETY: All `&self` methods on this type are written to ensure that it is safe to call them in
> -// parallel.
> -unsafe impl<T> Sync for MiscDeviceRegistration<T> {}
> +// SAFETY:
> +// - It is allowed to call `misc_deregister` on a different thread from where you called
> +//   `misc_register`.
> +// - Only implements `Send` if `MiscDevice::RegistrationData` is also `Send`.
> +unsafe impl<T: MiscDevice> Send for MiscDeviceRegistration<T> where T::RegistrationData: Send {}
> +
> +// SAFETY:
> +// - All `&self` methods on this type are written to ensure that it is safe to call them in
> +//   parallel.
> +// - `MiscDevice::RegistrationData` is always `Sync`.
> +unsafe impl<T: MiscDevice> Sync for MiscDeviceRegistration<T> {}

I would feel better if we still add the `T::RegistrationData: Sync`
bound here even if it is vacuous today.

>  impl<T: MiscDevice> MiscDeviceRegistration<T> {
>      /// Register a misc device.
> -    pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
> +    pub fn register(
> +        opts: MiscDeviceOptions,
> +        data: impl PinInit<T::RegistrationData, Error>,
> +    ) -> impl PinInit<Self, Error> {
>          try_pin_init!(Self {
> +            data <- Opaque::pin_init(data),
>              inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
>                  // SAFETY: The initializer can write to the provided `slot`.
>                  unsafe { slot.write(opts.into_raw::<T>()) };
>  
> -                // SAFETY: We just wrote the misc device options to the slot. The miscdevice will
> -                // get unregistered before `slot` is deallocated because the memory is pinned and
> -                // the destructor of this type deallocates the memory.
> +                // SAFETY:
> +                // * We just wrote the misc device options to the slot. The miscdevice will
> +                //   get unregistered before `slot` is deallocated because the memory is pinned and
> +                //   the destructor of this type deallocates the memory.
> +                // * `data` is Initialized before `misc_register` so no race with `fops->open()`
> +                //   is possible.
>                  // INVARIANT: If this returns `Ok(())`, then the `slot` will contain a registered
>                  // misc device.
>                  to_result(unsafe { bindings::misc_register(slot) })
> @@ -93,13 +108,24 @@ pub fn device(&self) -> &Device {
>          // before the underlying `struct miscdevice` is destroyed.
>          unsafe { Device::as_ref((*self.as_raw()).this_device) }
>      }
> +
> +    /// Access the additional data stored in this registration.
> +    pub fn data(&self) -> &T::RegistrationData {
> +        // SAFETY:
> +        // * No mutable reference to the value contained by `self.data` can ever be created.
> +        // * The value contained by `self.data` is valid for the entire lifetime of `&self`.

Please add type invariants for these two requirements.

> +        unsafe { &*self.data.get() }
> +    }
>  }
>  
>  #[pinned_drop]
> -impl<T> PinnedDrop for MiscDeviceRegistration<T> {
> +impl<T: MiscDevice> PinnedDrop for MiscDeviceRegistration<T> {
>      fn drop(self: Pin<&mut Self>) {
>          // SAFETY: We know that the device is registered by the type invariants.
>          unsafe { bindings::misc_deregister(self.inner.get()) };
> +
> +        // SAFETY: `self.data` is valid for dropping and nothing uses it anymore.

Ditto.

> +        unsafe { core::ptr::drop_in_place(self.data.get()) };
>      }
>  }
>  
> @@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
>      /// What kind of pointer should `Self` be wrapped in.
>      type Ptr: ForeignOwnable + Send + Sync;
>  
> +    /// The additional data carried by the [`MiscDeviceRegistration`] for this [`MiscDevice`].
> +    /// If no additional data is required than the unit type `()` should be used.
> +    ///
> +    /// This data can be accessed in [`MiscDevice::open()`] using
> +    /// [`MiscDeviceRegistration::data()`].
> +    type RegistrationData: Sync;

Why do we require `Sync` here?

We might want to give this a shorter name?

---
Cheers,
Benno

> +
>      /// Called when the misc device is opened.
>      ///
>      /// The returned pointer will be stored as the private data for the file.

Re: [PATCH v4 2/3] rust: miscdevice: add additional data to MiscDeviceRegistration

[Christian Schrefl]

On 31.05.25 2:23 PM, Benno Lossin wrote:
> On Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
>> @@ -45,32 +46,46 @@ pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
>>  /// # Invariants
>>  ///
>>  /// `inner` is a registered misc device.
>> -#[repr(transparent)]
>> +#[repr(C)]
> 
> Why do we need linear layout? `container_of!` also works with the `Rust`
> layout.

That was a leftover from a previous version, fixed.

> 
>>  #[pin_data(PinnedDrop)]
>> -pub struct MiscDeviceRegistration<T> {
>> +pub struct MiscDeviceRegistration<T: MiscDevice> {
>>      #[pin]
>>      inner: Opaque<bindings::miscdevice>,
>> +    #[pin]
>> +    data: Opaque<T::RegistrationData>,
>>      _t: PhantomData<T>,
> 
> No need to keep the `PhantomData` field around, since you're using `T`
> above.
> 

Fixed.

>>  }
>>  
>> -// SAFETY: It is allowed to call `misc_deregister` on a different thread from where you called
>> -// `misc_register`.
>> -unsafe impl<T> Send for MiscDeviceRegistration<T> {}
>> -// SAFETY: All `&self` methods on this type are written to ensure that it is safe to call them in
>> -// parallel.
>> -unsafe impl<T> Sync for MiscDeviceRegistration<T> {}
>> +// SAFETY:
>> +// - It is allowed to call `misc_deregister` on a different thread from where you called
>> +//   `misc_register`.
>> +// - Only implements `Send` if `MiscDevice::RegistrationData` is also `Send`.
>> +unsafe impl<T: MiscDevice> Send for MiscDeviceRegistration<T> where T::RegistrationData: Send {}
>> +
>> +// SAFETY:
>> +// - All `&self` methods on this type are written to ensure that it is safe to call them in
>> +//   parallel.
>> +// - `MiscDevice::RegistrationData` is always `Sync`.
>> +unsafe impl<T: MiscDevice> Sync for MiscDeviceRegistration<T> {}
> 
> I would feel better if we still add the `T::RegistrationData: Sync`
> bound here even if it is vacuous today.

Since a reference the `MiscDeviceRegistration` struct is an
argument to the open function this struct must always be Sync,
so adding bounds here doesn't make much sense.

I'll add this a safety comment in `MiscdeviceVTable::open`
about this.

Is there a good way to assert this at build to avoid regessions?
> 
>>  impl<T: MiscDevice> MiscDeviceRegistration<T> {
>>      /// Register a misc device.
>> -    pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
>> +    pub fn register(
>> +        opts: MiscDeviceOptions,
>> +        data: impl PinInit<T::RegistrationData, Error>,
>> +    ) -> impl PinInit<Self, Error> {
>>          try_pin_init!(Self {
>> +            data <- Opaque::pin_init(data),
>>              inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
>>                  // SAFETY: The initializer can write to the provided `slot`.
>>                  unsafe { slot.write(opts.into_raw::<T>()) };
>>  
>> -                // SAFETY: We just wrote the misc device options to the slot. The miscdevice will
>> -                // get unregistered before `slot` is deallocated because the memory is pinned and
>> -                // the destructor of this type deallocates the memory.
>> +                // SAFETY:
>> +                // * We just wrote the misc device options to the slot. The miscdevice will
>> +                //   get unregistered before `slot` is deallocated because the memory is pinned and
>> +                //   the destructor of this type deallocates the memory.
>> +                // * `data` is Initialized before `misc_register` so no race with `fops->open()`
>> +                //   is possible.
>>                  // INVARIANT: If this returns `Ok(())`, then the `slot` will contain a registered
>>                  // misc device.
>>                  to_result(unsafe { bindings::misc_register(slot) })
>> @@ -93,13 +108,24 @@ pub fn device(&self) -> &Device {
>>          // before the underlying `struct miscdevice` is destroyed.
>>          unsafe { Device::as_ref((*self.as_raw()).this_device) }
>>      }
>> +
>> +    /// Access the additional data stored in this registration.
>> +    pub fn data(&self) -> &T::RegistrationData {
>> +        // SAFETY:
>> +        // * No mutable reference to the value contained by `self.data` can ever be created.
>> +        // * The value contained by `self.data` is valid for the entire lifetime of `&self`.
> 
> Please add type invariants for these two requirements.
> 
>> +        unsafe { &*self.data.get() }
>> +    }
>>  }
>>  
>>  #[pinned_drop]
>> -impl<T> PinnedDrop for MiscDeviceRegistration<T> {
>> +impl<T: MiscDevice> PinnedDrop for MiscDeviceRegistration<T> {
>>      fn drop(self: Pin<&mut Self>) {
>>          // SAFETY: We know that the device is registered by the type invariants.
>>          unsafe { bindings::misc_deregister(self.inner.get()) };
>> +
>> +        // SAFETY: `self.data` is valid for dropping and nothing uses it anymore.
> 
> Ditto.

I'm not quite sure how to formulate these, what do you think of:

/// - `inner` is a registered misc device.
/// - `data` contains a valid `T::RegistrationData` for the whole lifetime of [`MiscDeviceRegistration`]
/// - `data` must be usable until `misc_deregister` (called when dropped) has returned.
/// - no mutable references to `data` may be created.

> 
>> +        unsafe { core::ptr::drop_in_place(self.data.get()) };
>>      }
>>  }
>>  
>> @@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
>>      /// What kind of pointer should `Self` be wrapped in.
>>      type Ptr: ForeignOwnable + Send + Sync;
>>  
>> +    /// The additional data carried by the [`MiscDeviceRegistration`] for this [`MiscDevice`].
>> +    /// If no additional data is required than the unit type `()` should be used.
>> +    ///
>> +    /// This data can be accessed in [`MiscDevice::open()`] using
>> +    /// [`MiscDeviceRegistration::data()`].
>> +    type RegistrationData: Sync;
> 
> Why do we require `Sync` here?

Needed for `MiscDeviceRegistration` to be `Send`, see response above.

> 
> We might want to give this a shorter name?

I think its fine, but I am open to Ideas.

Cheers
Christian

Re: [PATCH v4 2/3] rust: miscdevice: add additional data to MiscDeviceRegistration

[Benno Lossin]

On Mon Jun 2, 2025 at 11:16 PM CEST, Christian Schrefl wrote:
> On 31.05.25 2:23 PM, Benno Lossin wrote:
>> On Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
>>> +// SAFETY:
>>> +// - All `&self` methods on this type are written to ensure that it is safe to call them in
>>> +//   parallel.
>>> +// - `MiscDevice::RegistrationData` is always `Sync`.
>>> +unsafe impl<T: MiscDevice> Sync for MiscDeviceRegistration<T> {}
>> 
>> I would feel better if we still add the `T::RegistrationData: Sync`
>> bound here even if it is vacuous today.
>
> Since a reference the `MiscDeviceRegistration` struct is an
> argument to the open function this struct must always be Sync,
> so adding bounds here doesn't make much sense.

Well yes, but this statement makes `MiscDeviceRegistration` be `Sync`
even if `T::RegistrationData` is not `Sync` if that bound got removed
at some point. And this "instability" is what I'm worried about.

> I'll add this a safety comment in `MiscdeviceVTable::open`
> about this.
>
> Is there a good way to assert this at build to avoid regessions?

    const _: () = {
        fn assert_sync<T: ?Sized + Sync>() {}
        fn ctx<T: MiscDevice>() {
            assert_sync::<T::RegistrationData>();
        }
    };

That would also be fine with me if you insist on not adding the bound.

(the `assert_sync` function should maybe be somewhere where everyone can
use it)

>>>  impl<T: MiscDevice> MiscDeviceRegistration<T> {
>>>      /// Register a misc device.
>>> -    pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
>>> +    pub fn register(
>>> +        opts: MiscDeviceOptions,
>>> +        data: impl PinInit<T::RegistrationData, Error>,
>>> +    ) -> impl PinInit<Self, Error> {
>>>          try_pin_init!(Self {
>>> +            data <- Opaque::pin_init(data),
>>>              inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
>>>                  // SAFETY: The initializer can write to the provided `slot`.
>>>                  unsafe { slot.write(opts.into_raw::<T>()) };
>>>  
>>> -                // SAFETY: We just wrote the misc device options to the slot. The miscdevice will
>>> -                // get unregistered before `slot` is deallocated because the memory is pinned and
>>> -                // the destructor of this type deallocates the memory.
>>> +                // SAFETY:
>>> +                // * We just wrote the misc device options to the slot. The miscdevice will
>>> +                //   get unregistered before `slot` is deallocated because the memory is pinned and
>>> +                //   the destructor of this type deallocates the memory.
>>> +                // * `data` is Initialized before `misc_register` so no race with `fops->open()`
>>> +                //   is possible.
>>>                  // INVARIANT: If this returns `Ok(())`, then the `slot` will contain a registered
>>>                  // misc device.
>>>                  to_result(unsafe { bindings::misc_register(slot) })
>>> @@ -93,13 +108,24 @@ pub fn device(&self) -> &Device {
>>>          // before the underlying `struct miscdevice` is destroyed.
>>>          unsafe { Device::as_ref((*self.as_raw()).this_device) }
>>>      }
>>> +
>>> +    /// Access the additional data stored in this registration.
>>> +    pub fn data(&self) -> &T::RegistrationData {
>>> +        // SAFETY:
>>> +        // * No mutable reference to the value contained by `self.data` can ever be created.
>>> +        // * The value contained by `self.data` is valid for the entire lifetime of `&self`.
>> 
>> Please add type invariants for these two requirements.
>> 
>>> +        unsafe { &*self.data.get() }
>>> +    }
>>>  }
>>>  
>>>  #[pinned_drop]
>>> -impl<T> PinnedDrop for MiscDeviceRegistration<T> {
>>> +impl<T: MiscDevice> PinnedDrop for MiscDeviceRegistration<T> {
>>>      fn drop(self: Pin<&mut Self>) {
>>>          // SAFETY: We know that the device is registered by the type invariants.
>>>          unsafe { bindings::misc_deregister(self.inner.get()) };
>>> +
>>> +        // SAFETY: `self.data` is valid for dropping and nothing uses it anymore.
>> 
>> Ditto.
>
> I'm not quite sure how to formulate these, what do you think of:
>
> /// - `inner` is a registered misc device.

This doesn't really mean something to me, maybe it's better to reference
the registering function?

> /// - `data` contains a valid `T::RegistrationData` for the whole lifetime of [`MiscDeviceRegistration`]

This sounds good. But help me understand, why do we need `Opaque` /
`UnsafePinned` again? If we're only using shared references, then we
could also just store the object by value?

> /// - `data` must be usable until `misc_deregister` (called when dropped) has returned.

What does "usable" mean?

> /// - no mutable references to `data` may be created.

>>> +        unsafe { core::ptr::drop_in_place(self.data.get()) };
>>>      }
>>>  }
>>>  
>>> @@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
>>>      /// What kind of pointer should `Self` be wrapped in.
>>>      type Ptr: ForeignOwnable + Send + Sync;
>>>  
>>> +    /// The additional data carried by the [`MiscDeviceRegistration`] for this [`MiscDevice`].
>>> +    /// If no additional data is required than the unit type `()` should be used.
>>> +    ///
>>> +    /// This data can be accessed in [`MiscDevice::open()`] using
>>> +    /// [`MiscDeviceRegistration::data()`].
>>> +    type RegistrationData: Sync;
>> 
>> Why do we require `Sync` here?
>
> Needed for `MiscDeviceRegistration` to be `Send`, see response above.

You could also just ask the type there to be `Sync`, then users will get
an error when they try to use `MiscDevice` in a way where
`RegistrationData` is required to be `Sync`.

>> We might want to give this a shorter name?
>
> I think its fine, but I am open to Ideas.

`Data`?

---
Cheers,
Benno

Re: [PATCH v4 2/3] rust: miscdevice: add additional data to MiscDeviceRegistration

[Miguel Ojeda]

On Wed, Jun 4, 2025 at 1:29 AM Benno Lossin <lossin@kernel.org> wrote:
>
> (the `assert_sync` function should maybe be somewhere where everyone can
> use it)

+1, and likely part of the prelude too.

(We may want to put all these into an `assert.rs` or similar too.)

Cheers,
Miguel

Re: [PATCH v4 2/3] rust: miscdevice: add additional data to MiscDeviceRegistration

[Christian Schrefl]

On 04.06.25 10:48 AM, Miguel Ojeda wrote:
> On Wed, Jun 4, 2025 at 1:29 AM Benno Lossin <lossin@kernel.org> wrote:
>>
>> (the `assert_sync` function should maybe be somewhere where everyone can
>> use it)
> 
> +1, and likely part of the prelude too.
> 
> (We may want to put all these into an `assert.rs` or similar too.)

I think that we can add it to `build_assert.rs`, since this would
be a build time construct.

Should I do this in a separate series?

Cheers
Christian

Re: [PATCH v4 2/3] rust: miscdevice: add additional data to MiscDeviceRegistration

[Miguel Ojeda]

On Wed, Jun 4, 2025 at 11:54 AM Christian Schrefl
<chrisi.schrefl@gmail.com> wrote:
>
> I think that we can add it to `build_assert.rs`, since this would
> be a build time construct.

Hmm... it definitely makes sense, but I am not sure we want to mix
them -- the `build_assert!` is fairly special / different from the
others, since it is not really at compile-time like the others (which
is why it is a last-resort option), and it would be nice to have its
implementation self-contained in a different mod/file too.

Perhaps `compile_asserts.rs` or similar if we want to split them into
compiletime/buildtime/runtime?

> Should I do this in a separate series?

Yeah, no worries, it can be done later.

Thanks!

Cheers,
Miguel

Re: [PATCH v4 2/3] rust: miscdevice: add additional data to MiscDeviceRegistration

[Christian Schrefl]

On 04.06.25 1:29 AM, Benno Lossin wrote:
> On Mon Jun 2, 2025 at 11:16 PM CEST, Christian Schrefl wrote:
>> On 31.05.25 2:23 PM, Benno Lossin wrote:
>>> On Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
>>>> +// SAFETY:
>>>> +// - All `&self` methods on this type are written to ensure that it is safe to call them in
>>>> +//   parallel.
>>>> +// - `MiscDevice::RegistrationData` is always `Sync`.
>>>> +unsafe impl<T: MiscDevice> Sync for MiscDeviceRegistration<T> {}
>>>
>>> I would feel better if we still add the `T::RegistrationData: Sync`
>>> bound here even if it is vacuous today.
>>
>> Since a reference the `MiscDeviceRegistration` struct is an
>> argument to the open function this struct must always be Sync,
>> so adding bounds here doesn't make much sense.
> 
> Well yes, but this statement makes `MiscDeviceRegistration` be `Sync`
> even if `T::RegistrationData` is not `Sync` if that bound got removed
> at some point. And this "instability" is what I'm worried about.
> 
>> I'll add this a safety comment in `MiscdeviceVTable::open`
>> about this.
>>
>> Is there a good way to assert this at build to avoid regessions?
> 
>     const _: () = {
>         fn assert_sync<T: ?Sized + Sync>() {}
>         fn ctx<T: MiscDevice>() {
>             assert_sync::<T::RegistrationData>();
>         }
>     };
> 

I'll add the bound and a TODO about `assert_sync`, in `open`
where `Send` is required.

I intend to write a patch for `assert_sync` later.

> That would also be fine with me if you insist on not adding the bound.
> 
> (the `assert_sync` function should maybe be somewhere where everyone can
> use it)
> 
>>>>  impl<T: MiscDevice> MiscDeviceRegistration<T> {
>>>>      /// Register a misc device.
>>>> -    pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
>>>> +    pub fn register(
>>>> +        opts: MiscDeviceOptions,
>>>> +        data: impl PinInit<T::RegistrationData, Error>,
>>>> +    ) -> impl PinInit<Self, Error> {
>>>>          try_pin_init!(Self {
>>>> +            data <- Opaque::pin_init(data),
>>>>              inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
>>>>                  // SAFETY: The initializer can write to the provided `slot`.
>>>>                  unsafe { slot.write(opts.into_raw::<T>()) };
>>>>  
>>>> -                // SAFETY: We just wrote the misc device options to the slot. The miscdevice will
>>>> -                // get unregistered before `slot` is deallocated because the memory is pinned and
>>>> -                // the destructor of this type deallocates the memory.
>>>> +                // SAFETY:
>>>> +                // * We just wrote the misc device options to the slot. The miscdevice will
>>>> +                //   get unregistered before `slot` is deallocated because the memory is pinned and
>>>> +                //   the destructor of this type deallocates the memory.
>>>> +                // * `data` is Initialized before `misc_register` so no race with `fops->open()`
>>>> +                //   is possible.
>>>>                  // INVARIANT: If this returns `Ok(())`, then the `slot` will contain a registered
>>>>                  // misc device.
>>>>                  to_result(unsafe { bindings::misc_register(slot) })
>>>> @@ -93,13 +108,24 @@ pub fn device(&self) -> &Device {
>>>>          // before the underlying `struct miscdevice` is destroyed.
>>>>          unsafe { Device::as_ref((*self.as_raw()).this_device) }
>>>>      }
>>>> +
>>>> +    /// Access the additional data stored in this registration.
>>>> +    pub fn data(&self) -> &T::RegistrationData {
>>>> +        // SAFETY:
>>>> +        // * No mutable reference to the value contained by `self.data` can ever be created.
>>>> +        // * The value contained by `self.data` is valid for the entire lifetime of `&self`.
>>>
>>> Please add type invariants for these two requirements.
>>>
>>>> +        unsafe { &*self.data.get() }
>>>> +    }
>>>>  }
>>>>  
>>>>  #[pinned_drop]
>>>> -impl<T> PinnedDrop for MiscDeviceRegistration<T> {
>>>> +impl<T: MiscDevice> PinnedDrop for MiscDeviceRegistration<T> {
>>>>      fn drop(self: Pin<&mut Self>) {
>>>>          // SAFETY: We know that the device is registered by the type invariants.
>>>>          unsafe { bindings::misc_deregister(self.inner.get()) };
>>>> +
>>>> +        // SAFETY: `self.data` is valid for dropping and nothing uses it anymore.
>>>
>>> Ditto.
>>
>> I'm not quite sure how to formulate these, what do you think of:
>>
>> /// - `inner` is a registered misc device.
> 
> This doesn't really mean something to me, maybe it's better to reference
> the registering function?

That is from previous code so this should probably not be changed
in this series.

> 
>> /// - `data` contains a valid `T::RegistrationData` for the whole lifetime of [`MiscDeviceRegistration`]
> 
> This sounds good. But help me understand, why do we need `Opaque` /
> `UnsafePinned` again? If we're only using shared references, then we
> could also just store the object by value?

Since the Module owns the `MiscDeviceRegistration` it may create `&mut MiscDeviceRegistration`,
so from what I understand having a `& RegistrationData` reference into that is UB without
`UnsafePinned` (or `Opaque` since that includes `UnsafePinned` semantics).

> 
>> /// - `data` must be usable until `misc_deregister` (called when dropped) has returned.
> 
> What does "usable" mean?

I guess valid / alive might be better wording?

I meant to say that the `fops` functions might use the `RegistrationData` until 
`misc_deregister` has returned so we must ensure that these accesses are allowed.  

> 
>> /// - no mutable references to `data` may be created.
> 
>>>> +        unsafe { core::ptr::drop_in_place(self.data.get()) };
>>>>      }
>>>>  }
>>>>  
>>>> @@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
>>>>      /// What kind of pointer should `Self` be wrapped in.
>>>>      type Ptr: ForeignOwnable + Send + Sync;
>>>>  
>>>> +    /// The additional data carried by the [`MiscDeviceRegistration`] for this [`MiscDevice`].
>>>> +    /// If no additional data is required than the unit type `()` should be used.
>>>> +    ///
>>>> +    /// This data can be accessed in [`MiscDevice::open()`] using
>>>> +    /// [`MiscDeviceRegistration::data()`].
>>>> +    type RegistrationData: Sync;
>>>
>>> Why do we require `Sync` here?
>>
>> Needed for `MiscDeviceRegistration` to be `Send`, see response above.
> 
> You could also just ask the type there to be `Sync`, then users will get
> an error when they try to use `MiscDevice` in a way where
> `RegistrationData` is required to be `Sync`.

I don't think there is any point to allow defining a `MiscDevice` implementation
that cant actually be used/registered.

> 
>>> We might want to give this a shorter name?
>>
>> I think its fine, but I am open to Ideas.
> 
> `Data`?

I feel that `Data` is just very ambiguous, especially since it is associated with 
`MiscDevice` not the `MiscDeviceRegistration` in which its used.

One Idea I've had was `AssociatedData` but that's less clear and not much shorter
than `RegistrationData`.

But I'd be alright to just with `Data` if that is wanted.

Cheers
Christian

Re: [PATCH v4 2/3] rust: miscdevice: add additional data to MiscDeviceRegistration

[Benno Lossin]

On Thu Jun 5, 2025 at 4:57 PM CEST, Christian Schrefl wrote:
> On 04.06.25 1:29 AM, Benno Lossin wrote:
>> On Mon Jun 2, 2025 at 11:16 PM CEST, Christian Schrefl wrote:
>>> On 31.05.25 2:23 PM, Benno Lossin wrote:
>>>> On Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
>>>>> +// SAFETY:
>>>>> +// - All `&self` methods on this type are written to ensure that it is safe to call them in
>>>>> +//   parallel.
>>>>> +// - `MiscDevice::RegistrationData` is always `Sync`.
>>>>> +unsafe impl<T: MiscDevice> Sync for MiscDeviceRegistration<T> {}
>>>>
>>>> I would feel better if we still add the `T::RegistrationData: Sync`
>>>> bound here even if it is vacuous today.
>>>
>>> Since a reference the `MiscDeviceRegistration` struct is an
>>> argument to the open function this struct must always be Sync,
>>> so adding bounds here doesn't make much sense.
>> 
>> Well yes, but this statement makes `MiscDeviceRegistration` be `Sync`
>> even if `T::RegistrationData` is not `Sync` if that bound got removed
>> at some point. And this "instability" is what I'm worried about.
>> 
>>> I'll add this a safety comment in `MiscdeviceVTable::open`
>>> about this.
>>>
>>> Is there a good way to assert this at build to avoid regessions?
>> 
>>     const _: () = {
>>         fn assert_sync<T: ?Sized + Sync>() {}
>>         fn ctx<T: MiscDevice>() {
>>             assert_sync::<T::RegistrationData>();
>>         }
>>     };
>> 
>
> I'll add the bound and a TODO about `assert_sync`, in `open`
> where `Send` is required.
>
> I intend to write a patch for `assert_sync` later.

Great :)

>> That would also be fine with me if you insist on not adding the bound.
>> 
>> (the `assert_sync` function should maybe be somewhere where everyone can
>> use it)
>> 
>>>>>  impl<T: MiscDevice> MiscDeviceRegistration<T> {
>>>>>      /// Register a misc device.
>>>>> -    pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
>>>>> +    pub fn register(
>>>>> +        opts: MiscDeviceOptions,
>>>>> +        data: impl PinInit<T::RegistrationData, Error>,
>>>>> +    ) -> impl PinInit<Self, Error> {
>>>>>          try_pin_init!(Self {
>>>>> +            data <- Opaque::pin_init(data),
>>>>>              inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
>>>>>                  // SAFETY: The initializer can write to the provided `slot`.
>>>>>                  unsafe { slot.write(opts.into_raw::<T>()) };
>>>>>  
>>>>> -                // SAFETY: We just wrote the misc device options to the slot. The miscdevice will
>>>>> -                // get unregistered before `slot` is deallocated because the memory is pinned and
>>>>> -                // the destructor of this type deallocates the memory.
>>>>> +                // SAFETY:
>>>>> +                // * We just wrote the misc device options to the slot. The miscdevice will
>>>>> +                //   get unregistered before `slot` is deallocated because the memory is pinned and
>>>>> +                //   the destructor of this type deallocates the memory.
>>>>> +                // * `data` is Initialized before `misc_register` so no race with `fops->open()`
>>>>> +                //   is possible.
>>>>>                  // INVARIANT: If this returns `Ok(())`, then the `slot` will contain a registered
>>>>>                  // misc device.
>>>>>                  to_result(unsafe { bindings::misc_register(slot) })
>>>>> @@ -93,13 +108,24 @@ pub fn device(&self) -> &Device {
>>>>>          // before the underlying `struct miscdevice` is destroyed.
>>>>>          unsafe { Device::as_ref((*self.as_raw()).this_device) }
>>>>>      }
>>>>> +
>>>>> +    /// Access the additional data stored in this registration.
>>>>> +    pub fn data(&self) -> &T::RegistrationData {
>>>>> +        // SAFETY:
>>>>> +        // * No mutable reference to the value contained by `self.data` can ever be created.
>>>>> +        // * The value contained by `self.data` is valid for the entire lifetime of `&self`.
>>>>
>>>> Please add type invariants for these two requirements.
>>>>
>>>>> +        unsafe { &*self.data.get() }
>>>>> +    }
>>>>>  }
>>>>>  
>>>>>  #[pinned_drop]
>>>>> -impl<T> PinnedDrop for MiscDeviceRegistration<T> {
>>>>> +impl<T: MiscDevice> PinnedDrop for MiscDeviceRegistration<T> {
>>>>>      fn drop(self: Pin<&mut Self>) {
>>>>>          // SAFETY: We know that the device is registered by the type invariants.
>>>>>          unsafe { bindings::misc_deregister(self.inner.get()) };
>>>>> +
>>>>> +        // SAFETY: `self.data` is valid for dropping and nothing uses it anymore.
>>>>
>>>> Ditto.
>>>
>>> I'm not quite sure how to formulate these, what do you think of:
>>>
>>> /// - `inner` is a registered misc device.
>> 
>> This doesn't really mean something to me, maybe it's better to reference
>> the registering function?
>
> That is from previous code so this should probably not be changed
> in this series.

I personally wouldn't mind a commit that fixes this up, but if you don't
want to do it, let me know then we can make this a good-first-issue.

>>> /// - `data` contains a valid `T::RegistrationData` for the whole lifetime of [`MiscDeviceRegistration`]
>> 
>> This sounds good. But help me understand, why do we need `Opaque` /
>> `UnsafePinned` again? If we're only using shared references, then we
>> could also just store the object by value?
>
> Since the Module owns the `MiscDeviceRegistration` it may create `&mut MiscDeviceRegistration`,
> so from what I understand having a `& RegistrationData` reference into that is UB without
> `UnsafePinned` (or `Opaque` since that includes `UnsafePinned` semantics).

And the stored `T::RegistrationData` is shared as read-only with the C
side? Yes in that case we want `UnsafePinned<UnsafeCell<>>` (or for the
moment `Opaque`).

>>> /// - `data` must be usable until `misc_deregister` (called when dropped) has returned.
>> 
>> What does "usable" mean?
>
> I guess valid / alive might be better wording?
>
> I meant to say that the `fops` functions might use the `RegistrationData` until 
> `misc_deregister` has returned so we must ensure that these accesses are allowed.  

Then use `valid`.

>>> /// - no mutable references to `data` may be created.
>> 
>>>>> +        unsafe { core::ptr::drop_in_place(self.data.get()) };
>>>>>      }
>>>>>  }
>>>>>  
>>>>> @@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
>>>>>      /// What kind of pointer should `Self` be wrapped in.
>>>>>      type Ptr: ForeignOwnable + Send + Sync;
>>>>>  
>>>>> +    /// The additional data carried by the [`MiscDeviceRegistration`] for this [`MiscDevice`].
>>>>> +    /// If no additional data is required than the unit type `()` should be used.
>>>>> +    ///
>>>>> +    /// This data can be accessed in [`MiscDevice::open()`] using
>>>>> +    /// [`MiscDeviceRegistration::data()`].
>>>>> +    type RegistrationData: Sync;
>>>>
>>>> Why do we require `Sync` here?
>>>
>>> Needed for `MiscDeviceRegistration` to be `Send`, see response above.
>> 
>> You could also just ask the type there to be `Sync`, then users will get
>> an error when they try to use `MiscDevice` in a way where
>> `RegistrationData` is required to be `Sync`.
>
> I don't think there is any point to allow defining a `MiscDevice` implementation
> that cant actually be used/registered.

Sure, but the bound asserting that it is `Sync` doesn't need to be here,
having it just on the `impl Sync for MiscDeviceRegistration` is good
enough. (though one could argue that people would get an earlier error
if it is already asserted here. I think we should have some general
guidelines here :)

>>>> We might want to give this a shorter name?
>>>
>>> I think its fine, but I am open to Ideas.
>> 
>> `Data`?
>
> I feel that `Data` is just very ambiguous, especially since it is associated with 
> `MiscDevice` not the `MiscDeviceRegistration` in which its used.

But it is the data of the MiscDevice, no?

> One Idea I've had was `AssociatedData` but that's less clear and not much shorter
> than `RegistrationData`.

Of the two, I'd prefer `RegistrationData`.

> But I'd be alright to just with `Data` if that is wanted.

If you think that `RegistrationData` is more clear then go with that.
But I honestly don't derive much meaning from that over just `Data`. You
can still of course mention in the docs that this data is stored in the
registration.

But since there is no other way to associate data to a `MiscDevice`, I
think it makes sense to call it `Data`.

---
Cheers,
Benno

Re: [PATCH v4 2/3] rust: miscdevice: add additional data to MiscDeviceRegistration

[Christian Schrefl]

On 05.06.25 6:05 PM, Benno Lossin wrote:
> On Thu Jun 5, 2025 at 4:57 PM CEST, Christian Schrefl wrote:
>> On 04.06.25 1:29 AM, Benno Lossin wrote:
>>> On Mon Jun 2, 2025 at 11:16 PM CEST, Christian Schrefl wrote:
>>>> On 31.05.25 2:23 PM, Benno Lossin wrote:
>>>>> On Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
>>>>>> +// SAFETY:
>>>>>> +// - All `&self` methods on this type are written to ensure that it is safe to call them in
>>>>>> +//   parallel.
>>>>>> +// - `MiscDevice::RegistrationData` is always `Sync`.
>>>>>> +unsafe impl<T: MiscDevice> Sync for MiscDeviceRegistration<T> {}
>>>>>
>>>>> I would feel better if we still add the `T::RegistrationData: Sync`
>>>>> bound here even if it is vacuous today.
>>>>
>>>> Since a reference the `MiscDeviceRegistration` struct is an
>>>> argument to the open function this struct must always be Sync,
>>>> so adding bounds here doesn't make much sense.
>>>
>>> Well yes, but this statement makes `MiscDeviceRegistration` be `Sync`
>>> even if `T::RegistrationData` is not `Sync` if that bound got removed
>>> at some point. And this "instability" is what I'm worried about.
>>>
>>>> I'll add this a safety comment in `MiscdeviceVTable::open`
>>>> about this.
>>>>
>>>> Is there a good way to assert this at build to avoid regessions?
>>>
>>>     const _: () = {
>>>         fn assert_sync<T: ?Sized + Sync>() {}
>>>         fn ctx<T: MiscDevice>() {
>>>             assert_sync::<T::RegistrationData>();
>>>         }
>>>     };
>>>
>>
>> I'll add the bound and a TODO about `assert_sync`, in `open`
>> where `Send` is required.
>>
>> I intend to write a patch for `assert_sync` later.
> 
> Great :)
> 
>>> That would also be fine with me if you insist on not adding the bound.
>>>
>>> (the `assert_sync` function should maybe be somewhere where everyone can
>>> use it)
>>>
>>>>>>  impl<T: MiscDevice> MiscDeviceRegistration<T> {
>>>>>>      /// Register a misc device.
>>>>>> -    pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
>>>>>> +    pub fn register(
>>>>>> +        opts: MiscDeviceOptions,
>>>>>> +        data: impl PinInit<T::RegistrationData, Error>,
>>>>>> +    ) -> impl PinInit<Self, Error> {
>>>>>>          try_pin_init!(Self {
>>>>>> +            data <- Opaque::pin_init(data),
>>>>>>              inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
>>>>>>                  // SAFETY: The initializer can write to the provided `slot`.
>>>>>>                  unsafe { slot.write(opts.into_raw::<T>()) };
>>>>>>  
>>>>>> -                // SAFETY: We just wrote the misc device options to the slot. The miscdevice will
>>>>>> -                // get unregistered before `slot` is deallocated because the memory is pinned and
>>>>>> -                // the destructor of this type deallocates the memory.
>>>>>> +                // SAFETY:
>>>>>> +                // * We just wrote the misc device options to the slot. The miscdevice will
>>>>>> +                //   get unregistered before `slot` is deallocated because the memory is pinned and
>>>>>> +                //   the destructor of this type deallocates the memory.
>>>>>> +                // * `data` is Initialized before `misc_register` so no race with `fops->open()`
>>>>>> +                //   is possible.
>>>>>>                  // INVARIANT: If this returns `Ok(())`, then the `slot` will contain a registered
>>>>>>                  // misc device.
>>>>>>                  to_result(unsafe { bindings::misc_register(slot) })
>>>>>> @@ -93,13 +108,24 @@ pub fn device(&self) -> &Device {
>>>>>>          // before the underlying `struct miscdevice` is destroyed.
>>>>>>          unsafe { Device::as_ref((*self.as_raw()).this_device) }
>>>>>>      }
>>>>>> +
>>>>>> +    /// Access the additional data stored in this registration.
>>>>>> +    pub fn data(&self) -> &T::RegistrationData {
>>>>>> +        // SAFETY:
>>>>>> +        // * No mutable reference to the value contained by `self.data` can ever be created.
>>>>>> +        // * The value contained by `self.data` is valid for the entire lifetime of `&self`.
>>>>>
>>>>> Please add type invariants for these two requirements.
>>>>>
>>>>>> +        unsafe { &*self.data.get() }
>>>>>> +    }
>>>>>>  }
>>>>>>  
>>>>>>  #[pinned_drop]
>>>>>> -impl<T> PinnedDrop for MiscDeviceRegistration<T> {
>>>>>> +impl<T: MiscDevice> PinnedDrop for MiscDeviceRegistration<T> {
>>>>>>      fn drop(self: Pin<&mut Self>) {
>>>>>>          // SAFETY: We know that the device is registered by the type invariants.
>>>>>>          unsafe { bindings::misc_deregister(self.inner.get()) };
>>>>>> +
>>>>>> +        // SAFETY: `self.data` is valid for dropping and nothing uses it anymore.
>>>>>
>>>>> Ditto.
>>>>
>>>> I'm not quite sure how to formulate these, what do you think of:
>>>>
>>>> /// - `inner` is a registered misc device.
>>>
>>> This doesn't really mean something to me, maybe it's better to reference
>>> the registering function?
>>
>> That is from previous code so this should probably not be changed
>> in this series.
> 
> I personally wouldn't mind a commit that fixes this up, but if you don't
> want to do it, let me know then we can make this a good-first-issue.

I can do it, but I think it would make a good-first-issue so lets go
with that for now.

> 
>>>> /// - `data` contains a valid `T::RegistrationData` for the whole lifetime of [`MiscDeviceRegistration`]
>>>
>>> This sounds good. But help me understand, why do we need `Opaque` /
>>> `UnsafePinned` again? If we're only using shared references, then we
>>> could also just store the object by value?
>>
>> Since the Module owns the `MiscDeviceRegistration` it may create `&mut MiscDeviceRegistration`,
>> so from what I understand having a `& RegistrationData` reference into that is UB without
>> `UnsafePinned` (or `Opaque` since that includes `UnsafePinned` semantics).
> 
> And the stored `T::RegistrationData` is shared as read-only with the C
> side? Yes in that case we want `UnsafePinned<UnsafeCell<>>` (or for the
> moment `Opaque`).

Not really shared with the C side, but with the `open` implementation in
`MiscDevice` that is (indirectly) called by C. (`UnsafeCell` will probably not be
needed, as `UnsafePinned` will almost certainly have `UnsafeCell` semantics in upstream).

Thinking about this has made me realize that the current code already is a bit
iffy, since `MiscDevice::open` gets `&MiscDeviceRegistration<Self>` as an argument. (It
should be fine since `UnsafeCell` and `UnsafePinned` semantics also apply to "parrent" types
i.e. `&MiscDeviceRegistration` also has the semantics of `Opaque`).

> 
>>>> /// - `data` must be usable until `misc_deregister` (called when dropped) has returned.
>>>
>>> What does "usable" mean?
>>
>> I guess valid / alive might be better wording?
>>
>> I meant to say that the `fops` functions might use the `RegistrationData` until 
>> `misc_deregister` has returned so we must ensure that these accesses are allowed.  
> 
> Then use `valid`.

Alright.

> 
>>>> /// - no mutable references to `data` may be created.
>>>
>>>>>> +        unsafe { core::ptr::drop_in_place(self.data.get()) };
>>>>>>      }
>>>>>>  }
>>>>>>  
>>>>>> @@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
>>>>>>      /// What kind of pointer should `Self` be wrapped in.
>>>>>>      type Ptr: ForeignOwnable + Send + Sync;
>>>>>>  
>>>>>> +    /// The additional data carried by the [`MiscDeviceRegistration`] for this [`MiscDevice`].
>>>>>> +    /// If no additional data is required than the unit type `()` should be used.
>>>>>> +    ///
>>>>>> +    /// This data can be accessed in [`MiscDevice::open()`] using
>>>>>> +    /// [`MiscDeviceRegistration::data()`].
>>>>>> +    type RegistrationData: Sync;
>>>>>
>>>>> Why do we require `Sync` here?
>>>>
>>>> Needed for `MiscDeviceRegistration` to be `Send`, see response above.
>>>
>>> You could also just ask the type there to be `Sync`, then users will get
>>> an error when they try to use `MiscDevice` in a way where
>>> `RegistrationData` is required to be `Sync`.
>>
>> I don't think there is any point to allow defining a `MiscDevice` implementation
>> that cant actually be used/registered.
> 
> Sure, but the bound asserting that it is `Sync` doesn't need to be here,
> having it just on the `impl Sync for MiscDeviceRegistration` is good
> enough. (though one could argue that people would get an earlier error
> if it is already asserted here. I think we should have some general
> guidelines here :)

That would require a `Send` bound in the `register` function,
since a `MiscDevice` with `!Sync` `Data` would be valid now
(meaning that `MiscDeviceRegistration` may also be `!Sync`).

If you want I can go with that. I'm not really sure if its
really better (tough I don't feel that strongly either
way).

> 
>>>>> We might want to give this a shorter name?
>>>>
>>>> I think its fine, but I am open to Ideas.
>>>
>>> `Data`?
>>
>> I feel that `Data` is just very ambiguous, especially since it is associated with 
>> `MiscDevice` not the `MiscDeviceRegistration` in which its used.
> 
> But it is the data of the MiscDevice, no?
> 
>> One Idea I've had was `AssociatedData` but that's less clear and not much shorter
>> than `RegistrationData`.
> 
> Of the two, I'd prefer `RegistrationData`.
> 
>> But I'd be alright to just with `Data` if that is wanted.
> 
> If you think that `RegistrationData` is more clear then go with that.
> But I honestly don't derive much meaning from that over just `Data`. You
> can still of course mention in the docs that this data is stored in the
> registration.
> 
> But since there is no other way to associate data to a `MiscDevice`, I
> think it makes sense to call it `Data`.
> 

Alright I'll go with `Data` then.

> ---
> Cheers,
> Benno

Re: [PATCH v4 2/3] rust: miscdevice: add additional data to MiscDeviceRegistration

[Benno Lossin]

On Thu Jun 5, 2025 at 6:52 PM CEST, Christian Schrefl wrote:
> On 05.06.25 6:05 PM, Benno Lossin wrote:
>> On Thu Jun 5, 2025 at 4:57 PM CEST, Christian Schrefl wrote:
>>> On 04.06.25 1:29 AM, Benno Lossin wrote:
>>>> On Mon Jun 2, 2025 at 11:16 PM CEST, Christian Schrefl wrote:
>>>>> On 31.05.25 2:23 PM, Benno Lossin wrote:
>>>>>> On Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
>>>>>>>  #[pinned_drop]
>>>>>>> -impl<T> PinnedDrop for MiscDeviceRegistration<T> {
>>>>>>> +impl<T: MiscDevice> PinnedDrop for MiscDeviceRegistration<T> {
>>>>>>>      fn drop(self: Pin<&mut Self>) {
>>>>>>>          // SAFETY: We know that the device is registered by the type invariants.
>>>>>>>          unsafe { bindings::misc_deregister(self.inner.get()) };
>>>>>>> +
>>>>>>> +        // SAFETY: `self.data` is valid for dropping and nothing uses it anymore.
>>>>>>
>>>>>> Ditto.
>>>>>
>>>>> I'm not quite sure how to formulate these, what do you think of:
>>>>>
>>>>> /// - `inner` is a registered misc device.
>>>>
>>>> This doesn't really mean something to me, maybe it's better to reference
>>>> the registering function?
>>>
>>> That is from previous code so this should probably not be changed
>>> in this series.
>> 
>> I personally wouldn't mind a commit that fixes this up, but if you don't
>> want to do it, let me know then we can make this a good-first-issue.
>
> I can do it, but I think it would make a good-first-issue so lets go
> with that for now.

Feel free to open the issue :)

>>>>> /// - `data` contains a valid `T::RegistrationData` for the whole lifetime of [`MiscDeviceRegistration`]
>>>>
>>>> This sounds good. But help me understand, why do we need `Opaque` /
>>>> `UnsafePinned` again? If we're only using shared references, then we
>>>> could also just store the object by value?
>>>
>>> Since the Module owns the `MiscDeviceRegistration` it may create `&mut MiscDeviceRegistration`,
>>> so from what I understand having a `& RegistrationData` reference into that is UB without
>>> `UnsafePinned` (or `Opaque` since that includes `UnsafePinned` semantics).
>> 
>> And the stored `T::RegistrationData` is shared as read-only with the C
>> side? Yes in that case we want `UnsafePinned<UnsafeCell<>>` (or for the
>> moment `Opaque`).
>
> Not really shared with the C side, but with the `open` implementation in
> `MiscDevice` that is (indirectly) called by C. (`UnsafeCell` will probably not be
> needed, as `UnsafePinned` will almost certainly have `UnsafeCell` semantics in upstream).

Ah yes, I meant "shared with other Rust code through the C side" ie the
pointer round-trips through C (that isn't actually relevant, but that's
why I mentioned C).

> Thinking about this has made me realize that the current code already is a bit
> iffy, since `MiscDevice::open` gets `&MiscDeviceRegistration<Self>` as an argument. (It
> should be fine since `UnsafeCell` and `UnsafePinned` semantics also apply to "parrent" types
> i.e. `&MiscDeviceRegistration` also has the semantics of `Opaque`).

It's fine, since all non-ZST fields are `Opaque`. Otherwise we'd need to
wrap all fields with that.

>>>>> /// - no mutable references to `data` may be created.
>>>>
>>>>>>> +        unsafe { core::ptr::drop_in_place(self.data.get()) };
>>>>>>>      }
>>>>>>>  }
>>>>>>>  
>>>>>>> @@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
>>>>>>>      /// What kind of pointer should `Self` be wrapped in.
>>>>>>>      type Ptr: ForeignOwnable + Send + Sync;
>>>>>>>  
>>>>>>> +    /// The additional data carried by the [`MiscDeviceRegistration`] for this [`MiscDevice`].
>>>>>>> +    /// If no additional data is required than the unit type `()` should be used.
>>>>>>> +    ///
>>>>>>> +    /// This data can be accessed in [`MiscDevice::open()`] using
>>>>>>> +    /// [`MiscDeviceRegistration::data()`].
>>>>>>> +    type RegistrationData: Sync;
>>>>>>
>>>>>> Why do we require `Sync` here?
>>>>>
>>>>> Needed for `MiscDeviceRegistration` to be `Send`, see response above.
>>>>
>>>> You could also just ask the type there to be `Sync`, then users will get
>>>> an error when they try to use `MiscDevice` in a way where
>>>> `RegistrationData` is required to be `Sync`.
>>>
>>> I don't think there is any point to allow defining a `MiscDevice` implementation
>>> that cant actually be used/registered.
>> 
>> Sure, but the bound asserting that it is `Sync` doesn't need to be here,
>> having it just on the `impl Sync for MiscDeviceRegistration` is good
>> enough. (though one could argue that people would get an earlier error
>> if it is already asserted here. I think we should have some general
>> guidelines here :)
>
> That would require a `Send` bound in the `register` function,
> since a `MiscDevice` with `!Sync` `Data` would be valid now
> (meaning that `MiscDeviceRegistration` may also be `!Sync`).
>
> If you want I can go with that. I'm not really sure if its
> really better (tough I don't feel that strongly either
> way).

We don't lose anything by doing this, so I think we should do it.
If in the future someone invents a way `MiscDevice` that's only in the
current thread and it can be registered (so like a "thread-local"
`MiscDevice` :), then this will be less painful to change.

---
Cheers,
Benno

Re: [PATCH v4 2/3] rust: miscdevice: add additional data to MiscDeviceRegistration

[Christian Schrefl]

On 05.06.25 7:27 PM, Benno Lossin wrote:
> On Thu Jun 5, 2025 at 6:52 PM CEST, Christian Schrefl wrote:
>> On 05.06.25 6:05 PM, Benno Lossin wrote:
>>> On Thu Jun 5, 2025 at 4:57 PM CEST, Christian Schrefl wrote:
>>>> On 04.06.25 1:29 AM, Benno Lossin wrote:
>>>>> On Mon Jun 2, 2025 at 11:16 PM CEST, Christian Schrefl wrote:
>>>>>> On 31.05.25 2:23 PM, Benno Lossin wrote:
>>>>>>> On Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
>>>>>>>>  #[pinned_drop]
>>>>>>>> -impl<T> PinnedDrop for MiscDeviceRegistration<T> {
>>>>>>>> +impl<T: MiscDevice> PinnedDrop for MiscDeviceRegistration<T> {
>>>>>>>>      fn drop(self: Pin<&mut Self>) {
>>>>>>>>          // SAFETY: We know that the device is registered by the type invariants.
>>>>>>>>          unsafe { bindings::misc_deregister(self.inner.get()) };
>>>>>>>> +
>>>>>>>> +        // SAFETY: `self.data` is valid for dropping and nothing uses it anymore.
>>>>>>>
>>>>>>> Ditto.
>>>>>>
>>>>>> I'm not quite sure how to formulate these, what do you think of:
>>>>>>
>>>>>> /// - `inner` is a registered misc device.
>>>>>
>>>>> This doesn't really mean something to me, maybe it's better to reference
>>>>> the registering function?
>>>>
>>>> That is from previous code so this should probably not be changed
>>>> in this series.
>>>
>>> I personally wouldn't mind a commit that fixes this up, but if you don't
>>> want to do it, let me know then we can make this a good-first-issue.
>>
>> I can do it, but I think it would make a good-first-issue so lets go
>> with that for now.
> 
> Feel free to open the issue :)

I've opened [0]. I don't have the permissions to add tags for that.
[0]: https://github.com/Rust-for-Linux/linux/issues/1168

> 
>>>>>> /// - `data` contains a valid `T::RegistrationData` for the whole lifetime of [`MiscDeviceRegistration`]
>>>>>
>>>>> This sounds good. But help me understand, why do we need `Opaque` /
>>>>> `UnsafePinned` again? If we're only using shared references, then we
>>>>> could also just store the object by value?
>>>>
>>>> Since the Module owns the `MiscDeviceRegistration` it may create `&mut MiscDeviceRegistration`,
>>>> so from what I understand having a `& RegistrationData` reference into that is UB without
>>>> `UnsafePinned` (or `Opaque` since that includes `UnsafePinned` semantics).
>>>
>>> And the stored `T::RegistrationData` is shared as read-only with the C
>>> side? Yes in that case we want `UnsafePinned<UnsafeCell<>>` (or for the
>>> moment `Opaque`).
>>
>> Not really shared with the C side, but with the `open` implementation in
>> `MiscDevice` that is (indirectly) called by C. (`UnsafeCell` will probably not be
>> needed, as `UnsafePinned` will almost certainly have `UnsafeCell` semantics in upstream).
> 
> Ah yes, I meant "shared with other Rust code through the C side" ie the
> pointer round-trips through C (that isn't actually relevant, but that's
> why I mentioned C).
> 
>> Thinking about this has made me realize that the current code already is a bit
>> iffy, since `MiscDevice::open` gets `&MiscDeviceRegistration<Self>` as an argument. (It
>> should be fine since `UnsafeCell` and `UnsafePinned` semantics also apply to "parrent" types
>> i.e. `&MiscDeviceRegistration` also has the semantics of `Opaque`).
> 
> It's fine, since all non-ZST fields are `Opaque`. Otherwise we'd need to
> wrap all fields with that.

Yeah I understand that its not UB, but to me it seems a bit fragile and opaque why it is allowed.
That's what I meant by "a bit iffy".

> 
>>>>>> /// - no mutable references to `data` may be created.
>>>>>
>>>>>>>> +        unsafe { core::ptr::drop_in_place(self.data.get()) };
>>>>>>>>      }
>>>>>>>>  }
>>>>>>>>  
>>>>>>>> @@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
>>>>>>>>      /// What kind of pointer should `Self` be wrapped in.
>>>>>>>>      type Ptr: ForeignOwnable + Send + Sync;
>>>>>>>>  
>>>>>>>> +    /// The additional data carried by the [`MiscDeviceRegistration`] for this [`MiscDevice`].
>>>>>>>> +    /// If no additional data is required than the unit type `()` should be used.
>>>>>>>> +    ///
>>>>>>>> +    /// This data can be accessed in [`MiscDevice::open()`] using
>>>>>>>> +    /// [`MiscDeviceRegistration::data()`].
>>>>>>>> +    type RegistrationData: Sync;
>>>>>>>
>>>>>>> Why do we require `Sync` here?
>>>>>>
>>>>>> Needed for `MiscDeviceRegistration` to be `Send`, see response above.
>>>>>
>>>>> You could also just ask the type there to be `Sync`, then users will get
>>>>> an error when they try to use `MiscDevice` in a way where
>>>>> `RegistrationData` is required to be `Sync`.
>>>>
>>>> I don't think there is any point to allow defining a `MiscDevice` implementation
>>>> that cant actually be used/registered.
>>>
>>> Sure, but the bound asserting that it is `Sync` doesn't need to be here,
>>> having it just on the `impl Sync for MiscDeviceRegistration` is good
>>> enough. (though one could argue that people would get an earlier error
>>> if it is already asserted here. I think we should have some general
>>> guidelines here :)
>>
>> That would require a `Send` bound in the `register` function,
>> since a `MiscDevice` with `!Sync` `Data` would be valid now
>> (meaning that `MiscDeviceRegistration` may also be `!Sync`).
>>
>> If you want I can go with that. I'm not really sure if its
>> really better (tough I don't feel that strongly either
>> way).
> 
> We don't lose anything by doing this, so I think we should do it.
> If in the future someone invents a way `MiscDevice` that's only in the
> current thread and it can be registered (so like a "thread-local"
> `MiscDevice` :), then this will be less painful to change.

Alright but I doubt that realistic, since the `Data` would always at
least be shared between the owner of `MiscDeviceRegistration` and the
`fops` implementation. Meaning its always shared with syscall context
and I don't think it makes sense to have a registration owed in 
that context.

Cheers
Christian

Re: [PATCH v4 2/3] rust: miscdevice: add additional data to MiscDeviceRegistration

[Benno Lossin]

On Sat Jun 7, 2025 at 1:34 PM CEST, Christian Schrefl wrote:
> Yeah I understand that its not UB, but to me it seems a bit fragile and opaque why it is allowed.
> That's what I meant by "a bit iffy".

What's fragile about it? That someone could add a non-opaque field to
the struct? Or that one is not allowed to take an `&`?

> Alright but I doubt that realistic, since the `Data` would always at
> least be shared between the owner of `MiscDeviceRegistration` and the
> `fops` implementation. Meaning its always shared with syscall context
> and I don't think it makes sense to have a registration owed in 
> that context.

That might be the case, but I think we should have this as a general
design guideline: avoid unnecessary trait bounds.

---
Cheers,
Benno

Re: [PATCH v4 2/3] rust: miscdevice: add additional data to MiscDeviceRegistration

[Christian Schrefl]

On 07.06.25 5:37 PM, Benno Lossin wrote:
> On Sat Jun 7, 2025 at 1:34 PM CEST, Christian Schrefl wrote:
>> Yeah I understand that its not UB, but to me it seems a bit fragile and opaque why it is allowed.
>> That's what I meant by "a bit iffy".
> 
> What's fragile about it? That someone could add a non-opaque field to
> the struct? Or that one is not allowed to take an `&`?

Yeah that a added field could cause UB seems bad to me.

> 
>> Alright but I doubt that realistic, since the `Data` would always at
>> least be shared between the owner of `MiscDeviceRegistration` and the
>> `fops` implementation. Meaning its always shared with syscall context
>> and I don't think it makes sense to have a registration owed in 
>> that context.
> 
> That might be the case, but I think we should have this as a general
> design guideline: avoid unnecessary trait bounds.

Alright seems fine to me.

Cheers
Christian

Re: [PATCH v4 2/3] rust: miscdevice: add additional data to MiscDeviceRegistration

[Benno Lossin]

On Sat Jun 7, 2025 at 5:39 PM CEST, Christian Schrefl wrote:
> On 07.06.25 5:37 PM, Benno Lossin wrote:
>> On Sat Jun 7, 2025 at 1:34 PM CEST, Christian Schrefl wrote:
>>> Yeah I understand that its not UB, but to me it seems a bit fragile and opaque why it is allowed.
>>> That's what I meant by "a bit iffy".
>> 
>> What's fragile about it? That someone could add a non-opaque field to
>> the struct? Or that one is not allowed to take an `&`?
>
> Yeah that a added field could cause UB seems bad to me.

Actually, I don't think it would be UB, since only the `data` field
would be borrowed mutably/immutably at the same time, but not the new
field.

---
Cheers,
Benno

Re: [PATCH v4 2/3] rust: miscdevice: add additional data to MiscDeviceRegistration

[Alice Ryhl]

On Mon, Jun 02, 2025 at 11:16:33PM +0200, Christian Schrefl wrote:
> On 31.05.25 2:23 PM, Benno Lossin wrote:
> > On Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
> >> +        unsafe { core::ptr::drop_in_place(self.data.get()) };
> >>      }
> >>  }
> >>  
> >> @@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
> >>      /// What kind of pointer should `Self` be wrapped in.
> >>      type Ptr: ForeignOwnable + Send + Sync;
> >>  
> >> +    /// The additional data carried by the [`MiscDeviceRegistration`] for this [`MiscDevice`].
> >> +    /// If no additional data is required than the unit type `()` should be used.
> >> +    ///
> >> +    /// This data can be accessed in [`MiscDevice::open()`] using
> >> +    /// [`MiscDeviceRegistration::data()`].
> >> +    type RegistrationData: Sync;
> > 
> > Why do we require `Sync` here?
> 
> Needed for `MiscDeviceRegistration` to be `Send`, see response above.

Even if `MiscDeviceRegistration` is non-Send, the registration data must
still be Sync. The f_ops->open callback can *always* be called from any
thread no matter what we do here, so the data it gives you access must
always be Sync no matter what.

Alice

Re: [PATCH v4 2/3] rust: miscdevice: add additional data to MiscDeviceRegistration

[Christian Schrefl]

On 04.06.25 11:40 AM, Alice Ryhl wrote:
> On Mon, Jun 02, 2025 at 11:16:33PM +0200, Christian Schrefl wrote:
>> On 31.05.25 2:23 PM, Benno Lossin wrote:
>>> On Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
>>>> +        unsafe { core::ptr::drop_in_place(self.data.get()) };
>>>>      }
>>>>  }
>>>>  
>>>> @@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
>>>>      /// What kind of pointer should `Self` be wrapped in.
>>>>      type Ptr: ForeignOwnable + Send + Sync;
>>>>  
>>>> +    /// The additional data carried by the [`MiscDeviceRegistration`] for this [`MiscDevice`].
>>>> +    /// If no additional data is required than the unit type `()` should be used.
>>>> +    ///
>>>> +    /// This data can be accessed in [`MiscDevice::open()`] using
>>>> +    /// [`MiscDeviceRegistration::data()`].
>>>> +    type RegistrationData: Sync;
>>>
>>> Why do we require `Sync` here?
>>
>> Needed for `MiscDeviceRegistration` to be `Send`, see response above.
> 
> Even if `MiscDeviceRegistration` is non-Send, the registration data must
> still be Sync. The f_ops->open callback can *always* be called from any
> thread no matter what we do here, so the data it gives you access must
> always be Sync no matter what.

Right I meant to write `Sync` sorry.

Cheers
Christian

Re: [PATCH v4 2/3] rust: miscdevice: add additional data to MiscDeviceRegistration

[Alice Ryhl]

On Wed, Jun 4, 2025 at 11:42 AM Christian Schrefl
<chrisi.schrefl@gmail.com> wrote:
>
> On 04.06.25 11:40 AM, Alice Ryhl wrote:
> > On Mon, Jun 02, 2025 at 11:16:33PM +0200, Christian Schrefl wrote:
> >> On 31.05.25 2:23 PM, Benno Lossin wrote:
> >>> On Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
> >>>> +        unsafe { core::ptr::drop_in_place(self.data.get()) };
> >>>>      }
> >>>>  }
> >>>>
> >>>> @@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
> >>>>      /// What kind of pointer should `Self` be wrapped in.
> >>>>      type Ptr: ForeignOwnable + Send + Sync;
> >>>>
> >>>> +    /// The additional data carried by the [`MiscDeviceRegistration`] for this [`MiscDevice`].
> >>>> +    /// If no additional data is required than the unit type `()` should be used.
> >>>> +    ///
> >>>> +    /// This data can be accessed in [`MiscDevice::open()`] using
> >>>> +    /// [`MiscDeviceRegistration::data()`].
> >>>> +    type RegistrationData: Sync;
> >>>
> >>> Why do we require `Sync` here?
> >>
> >> Needed for `MiscDeviceRegistration` to be `Send`, see response above.
> >
> > Even if `MiscDeviceRegistration` is non-Send, the registration data must
> > still be Sync. The f_ops->open callback can *always* be called from any
> > thread no matter what we do here, so the data it gives you access must
> > always be Sync no matter what.
>
> Right I meant to write `Sync` sorry.

It is still the case that RegistrationData must always be Sync no
matter what. But I guess that also applies to MiscDeviceRegistration.

Alice

Re: [PATCH v4 2/3] rust: miscdevice: add additional data to MiscDeviceRegistration

[Alice Ryhl]

On Sat, May 31, 2025 at 02:23:23PM +0200, Benno Lossin wrote:
> On Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
> > @@ -45,32 +46,46 @@ pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
> >  /// # Invariants
> >  ///
> >  /// `inner` is a registered misc device.
> > -#[repr(transparent)]
> > +#[repr(C)]
> 
> Why do we need linear layout? `container_of!` also works with the `Rust`
> layout.
> 
> >  #[pin_data(PinnedDrop)]
> > -pub struct MiscDeviceRegistration<T> {
> > +pub struct MiscDeviceRegistration<T: MiscDevice> {
> >      #[pin]
> >      inner: Opaque<bindings::miscdevice>,
> > +    #[pin]
> > +    data: Opaque<T::RegistrationData>,
> >      _t: PhantomData<T>,
> 
> No need to keep the `PhantomData` field around, since you're using `T`
> above.
> 
> >  }
> >  
> > -// SAFETY: It is allowed to call `misc_deregister` on a different thread from where you called
> > -// `misc_register`.
> > -unsafe impl<T> Send for MiscDeviceRegistration<T> {}
> > -// SAFETY: All `&self` methods on this type are written to ensure that it is safe to call them in
> > -// parallel.
> > -unsafe impl<T> Sync for MiscDeviceRegistration<T> {}
> > +// SAFETY:
> > +// - It is allowed to call `misc_deregister` on a different thread from where you called
> > +//   `misc_register`.
> > +// - Only implements `Send` if `MiscDevice::RegistrationData` is also `Send`.
> > +unsafe impl<T: MiscDevice> Send for MiscDeviceRegistration<T> where T::RegistrationData: Send {}
> > +
> > +// SAFETY:
> > +// - All `&self` methods on this type are written to ensure that it is safe to call them in
> > +//   parallel.
> > +// - `MiscDevice::RegistrationData` is always `Sync`.
> > +unsafe impl<T: MiscDevice> Sync for MiscDeviceRegistration<T> {}
> 
> I would feel better if we still add the `T::RegistrationData: Sync`
> bound here even if it is vacuous today.
> 
> >  impl<T: MiscDevice> MiscDeviceRegistration<T> {
> >      /// Register a misc device.
> > -    pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
> > +    pub fn register(
> > +        opts: MiscDeviceOptions,
> > +        data: impl PinInit<T::RegistrationData, Error>,
> > +    ) -> impl PinInit<Self, Error> {
> >          try_pin_init!(Self {
> > +            data <- Opaque::pin_init(data),
> >              inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
> >                  // SAFETY: The initializer can write to the provided `slot`.
> >                  unsafe { slot.write(opts.into_raw::<T>()) };
> >  
> > -                // SAFETY: We just wrote the misc device options to the slot. The miscdevice will
> > -                // get unregistered before `slot` is deallocated because the memory is pinned and
> > -                // the destructor of this type deallocates the memory.
> > +                // SAFETY:
> > +                // * We just wrote the misc device options to the slot. The miscdevice will
> > +                //   get unregistered before `slot` is deallocated because the memory is pinned and
> > +                //   the destructor of this type deallocates the memory.
> > +                // * `data` is Initialized before `misc_register` so no race with `fops->open()`
> > +                //   is possible.
> >                  // INVARIANT: If this returns `Ok(())`, then the `slot` will contain a registered
> >                  // misc device.
> >                  to_result(unsafe { bindings::misc_register(slot) })
> > @@ -93,13 +108,24 @@ pub fn device(&self) -> &Device {
> >          // before the underlying `struct miscdevice` is destroyed.
> >          unsafe { Device::as_ref((*self.as_raw()).this_device) }
> >      }
> > +
> > +    /// Access the additional data stored in this registration.
> > +    pub fn data(&self) -> &T::RegistrationData {
> > +        // SAFETY:
> > +        // * No mutable reference to the value contained by `self.data` can ever be created.
> > +        // * The value contained by `self.data` is valid for the entire lifetime of `&self`.
> 
> Please add type invariants for these two requirements.
> 
> > +        unsafe { &*self.data.get() }
> > +    }
> >  }
> >  
> >  #[pinned_drop]
> > -impl<T> PinnedDrop for MiscDeviceRegistration<T> {
> > +impl<T: MiscDevice> PinnedDrop for MiscDeviceRegistration<T> {
> >      fn drop(self: Pin<&mut Self>) {
> >          // SAFETY: We know that the device is registered by the type invariants.
> >          unsafe { bindings::misc_deregister(self.inner.get()) };
> > +
> > +        // SAFETY: `self.data` is valid for dropping and nothing uses it anymore.
> 
> Ditto.
> 
> > +        unsafe { core::ptr::drop_in_place(self.data.get()) };
> >      }
> >  }
> >  
> > @@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
> >      /// What kind of pointer should `Self` be wrapped in.
> >      type Ptr: ForeignOwnable + Send + Sync;
> >  
> > +    /// The additional data carried by the [`MiscDeviceRegistration`] for this [`MiscDevice`].
> > +    /// If no additional data is required than the unit type `()` should be used.
> > +    ///
> > +    /// This data can be accessed in [`MiscDevice::open()`] using
> > +    /// [`MiscDeviceRegistration::data()`].
> > +    type RegistrationData: Sync;
> 
> Why do we require `Sync` here?
> 
> We might want to give this a shorter name?

Re: [PATCH v4 2/3] rust: miscdevice: add additional data to MiscDeviceRegistration

[Alice Ryhl]

On Wed, Jun 4, 2025 at 11:37 AM Alice Ryhl <aliceryhl@google.com> wrote:
>

Please disregard this email. It was empty by mistake.

Alice

[PATCH v4 3/3] rust: miscdevice: adjust the rust_misc_device sample to use RegistrationData.

[Christian Schrefl]

Add a second mutex to the RustMiscDevice, which is shared between all
instances of the device using an Arc and the RegistrationData of
MiscDeviceRegistration.

This is mostly to demonstrate the capability to share data in this way.

Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
---
 samples/rust/rust_misc_device.rs | 120 +++++++++++++++++++++++++++++++++++----
 1 file changed, 110 insertions(+), 10 deletions(-)

diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
index 67a6172fbbf72dd42a1b655f5f5a782101432707..3c96cf8fe747427106f2e436c3dba33008c7fd53 100644
--- a/samples/rust/rust_misc_device.rs
+++ b/samples/rust/rust_misc_device.rs
@@ -18,6 +18,8 @@
 //! #define RUST_MISC_DEV_HELLO _IO('|', 0x80)
 //! #define RUST_MISC_DEV_GET_VALUE _IOR('|', 0x81, int)
 //! #define RUST_MISC_DEV_SET_VALUE _IOW('|', 0x82, int)
+//! #define RUST_MISC_DEV_GET_SHARED_VALUE _IOR('|', 0x83, int)
+//! #define RUST_MISC_DEV_SET_SHARED_VALUE _IOW('|', 0x84, int)
 //!
 //! int main() {
 //!   int value, new_value;
@@ -86,6 +88,62 @@
 //!     return -1;
 //!   }
 //!
+//!   value++;
+//!
+//!   // Set shared value to something different
+//!   printf("Submitting new shared value (%d)\n", value);
+//!   ret = ioctl(fd, RUST_MISC_DEV_SET_SHARED_VALUE, &value);
+//!   if (ret < 0) {
+//!     perror("ioctl: Failed to submit new value");
+//!     close(fd);
+//!     return errno;
+//!   }
+//!
+//!   // Close the device file
+//!   printf("Closing /dev/rust-misc-device\n");
+//!   close(fd);
+//!
+//!   // Open the device file again
+//!   printf("Opening /dev/rust-misc-device again for reading\n");
+//!   fd = open("/dev/rust-misc-device", O_RDWR);
+//!   if (fd < 0) {
+//!     perror("open");
+//!     return errno;
+//!   }
+//!
+//!   // Ensure new value was applied
+//!   printf("Fetching new value\n");
+//!   ret = ioctl(fd, RUST_MISC_DEV_GET_SHARED_VALUE, &new_value);
+//!   if (ret < 0) {
+//!     perror("ioctl: Failed to fetch the new value");
+//!     close(fd);
+//!     return errno;
+//!   }
+//!
+//!   if (value != new_value) {
+//!     printf("Failed: Committed and retrieved values are different (%d - %d)\n",
+//!            value, new_value);
+//!     close(fd);
+//!     return -1;
+//!   }
+//!
+//!   value = 0;
+//!   // Ensure non-shared value is still 0
+//!   printf("Fetching new value\n");
+//!   ret = ioctl(fd, RUST_MISC_DEV_GET_VALUE, &new_value);
+//!   if (ret < 0) {
+//!     perror("ioctl: Failed to fetch the new value");
+//!     close(fd);
+//!     return errno;
+//!   }
+//!
+//!   if (value != new_value) {
+//!     printf("Failed: Committed and retrieved values are different (%d - %d)\n",
+//!            value, new_value);
+//!     close(fd);
+//!     return -1;
+//!   }
+//!
 //!   // Close the device file
 //!   printf("Closing /dev/rust-misc-device\n");
 //!   close(fd);
@@ -94,7 +152,6 @@
 //!   return 0;
 //! }
 //! ```
-
 use core::pin::Pin;
 
 use kernel::{
@@ -105,7 +162,7 @@
     miscdevice::{MiscDevice, MiscDeviceOptions, MiscDeviceRegistration},
     new_mutex,
     prelude::*,
-    sync::Mutex,
+    sync::{Arc, Mutex},
     types::ARef,
     uaccess::{UserSlice, UserSliceReader, UserSliceWriter},
 };
@@ -113,6 +170,8 @@
 const RUST_MISC_DEV_HELLO: u32 = _IO('|' as u32, 0x80);
 const RUST_MISC_DEV_GET_VALUE: u32 = _IOR::<i32>('|' as u32, 0x81);
 const RUST_MISC_DEV_SET_VALUE: u32 = _IOW::<i32>('|' as u32, 0x82);
+const RUST_MISC_DEV_GET_SHARED_VALUE: u32 = _IOR::<i32>('|' as u32, 0x83);
+const RUST_MISC_DEV_SET_SHARED_VALUE: u32 = _IOW::<i32>('|' as u32, 0x84);
 
 module! {
     type: RustMiscDeviceModule,
@@ -130,14 +189,17 @@ struct RustMiscDeviceModule {
 
 impl kernel::InPlaceModule for RustMiscDeviceModule {
     fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
-        pr_info!("Initialising Rust Misc Device Sample\n");
+        pr_info!("Initializing Rust Misc Device Sample\n");
 
         let options = MiscDeviceOptions {
             name: c_str!("rust-misc-device"),
         };
 
         try_pin_init!(Self {
-            _miscdev <- MiscDeviceRegistration::register(options, ()),
+            _miscdev <- MiscDeviceRegistration::register(
+                options,
+                Arc::pin_init(new_mutex!(Inner { value: 0_i32 }), GFP_KERNEL)?
+            ),
         })
     }
 }
@@ -148,8 +210,9 @@ struct Inner {
 
 #[pin_data(PinnedDrop)]
 struct RustMiscDevice {
+    shared: Arc<Mutex<Inner>>,
     #[pin]
-    inner: Mutex<Inner>,
+    unique: Mutex<Inner>,
     dev: ARef<Device>,
 }
 
@@ -157,7 +220,7 @@ struct RustMiscDevice {
 impl MiscDevice for RustMiscDevice {
     type Ptr = Pin<KBox<Self>>;
 
-    type RegistrationData = ();
+    type RegistrationData = Arc<Mutex<Inner>>;
 
     fn open(_file: &File, misc: &MiscDeviceRegistration<Self>) -> Result<Pin<KBox<Self>>> {
         let dev = ARef::from(misc.device());
@@ -167,7 +230,8 @@ fn open(_file: &File, misc: &MiscDeviceRegistration<Self>) -> Result<Pin<KBox<Se
         KBox::try_pin_init(
             try_pin_init! {
                 RustMiscDevice {
-                    inner <- new_mutex!( Inner{ value: 0_i32 } ),
+                    shared: misc.data().clone(),
+                    unique <- new_mutex!(Inner { value: 0_i32 }),
                     dev: dev,
                 }
             },
@@ -183,6 +247,12 @@ fn ioctl(me: Pin<&RustMiscDevice>, _file: &File, cmd: u32, arg: usize) -> Result
         match cmd {
             RUST_MISC_DEV_GET_VALUE => me.get_value(UserSlice::new(arg, size).writer())?,
             RUST_MISC_DEV_SET_VALUE => me.set_value(UserSlice::new(arg, size).reader())?,
+            RUST_MISC_DEV_GET_SHARED_VALUE => {
+                me.get_shared_value(UserSlice::new(arg, size).writer())?
+            }
+            RUST_MISC_DEV_SET_SHARED_VALUE => {
+                me.set_shared_value(UserSlice::new(arg, size).reader())?
+            }
             RUST_MISC_DEV_HELLO => me.hello()?,
             _ => {
                 dev_err!(me.dev, "-> IOCTL not recognised: {}\n", cmd);
@@ -193,7 +263,6 @@ fn ioctl(me: Pin<&RustMiscDevice>, _file: &File, cmd: u32, arg: usize) -> Result
         Ok(0)
     }
 }
-
 #[pinned_drop]
 impl PinnedDrop for RustMiscDevice {
     fn drop(self: Pin<&mut Self>) {
@@ -204,7 +273,7 @@ fn drop(self: Pin<&mut Self>) {
 impl RustMiscDevice {
     fn set_value(&self, mut reader: UserSliceReader) -> Result<isize> {
         let new_value = reader.read::<i32>()?;
-        let mut guard = self.inner.lock();
+        let mut guard = self.unique.lock();
 
         dev_info!(
             self.dev,
@@ -217,7 +286,38 @@ fn set_value(&self, mut reader: UserSliceReader) -> Result<isize> {
     }
 
     fn get_value(&self, mut writer: UserSliceWriter) -> Result<isize> {
-        let guard = self.inner.lock();
+        let guard = self.unique.lock();
+        let value = guard.value;
+
+        // Free-up the lock and use our locally cached instance from here
+        drop(guard);
+
+        dev_info!(
+            self.dev,
+            "-> Copying data to userspace (value: {})\n",
+            &value
+        );
+
+        writer.write::<i32>(&value)?;
+        Ok(0)
+    }
+
+    fn set_shared_value(&self, mut reader: UserSliceReader) -> Result<isize> {
+        let new_value = reader.read::<i32>()?;
+        let mut guard = self.shared.lock();
+
+        dev_info!(
+            self.dev,
+            "-> Copying data from userspace (value: {})\n",
+            new_value
+        );
+
+        guard.value = new_value;
+        Ok(0)
+    }
+
+    fn get_shared_value(&self, mut writer: UserSliceWriter) -> Result<isize> {
+        let guard = self.shared.lock();
         let value = guard.value;
 
         // Free-up the lock and use our locally cached instance from here

-- 
2.49.0

Re: [PATCH v4 3/3] rust: miscdevice: adjust the rust_misc_device sample to use RegistrationData.

[Benno Lossin]

On Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
> Add a second mutex to the RustMiscDevice, which is shared between all
> instances of the device using an Arc and the RegistrationData of
> MiscDeviceRegistration.
>
> This is mostly to demonstrate the capability to share data in this way.
>
> Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>

Two nits below, with those fixed:

Reviewed-by: Benno Lossin <lossin@kernel.org>

> @@ -94,7 +152,6 @@
>  //!   return 0;
>  //! }
>  //! ```
> -

Let's keep this newline.

>  use core::pin::Pin;
>  
>  use kernel::{

> @@ -193,7 +263,6 @@ fn ioctl(me: Pin<&RustMiscDevice>, _file: &File, cmd: u32, arg: usize) -> Result
>          Ok(0)
>      }
>  }
> -

This one too.

---
Cheers,
Benno

>  #[pinned_drop]
>  impl PinnedDrop for RustMiscDevice {
>      fn drop(self: Pin<&mut Self>) {
> @@ -204,7 +273,7 @@ fn drop(self: Pin<&mut Self>) {
>  impl RustMiscDevice {
>      fn set_value(&self, mut reader: UserSliceReader) -> Result<isize> {
>          let new_value = reader.read::<i32>()?;
> -        let mut guard = self.inner.lock();
> +        let mut guard = self.unique.lock();
>  
>          dev_info!(
>              self.dev,

Re: [PATCH v4 3/3] rust: miscdevice: adjust the rust_misc_device sample to use RegistrationData.

[Miguel Ojeda]

On Sat, May 31, 2025 at 2:27 PM Benno Lossin <lossin@kernel.org> wrote:
>
> Let's keep this newline.

There is also another apparently spurious change:

> -        pr_info!("Initialising Rust Misc Device Sample\n");
> +        pr_info!("Initializing Rust Misc Device Sample\n");

In general, please try to avoid unrelated changes in patches.

Thanks!

Cheers,
Miguel

Re: [PATCH v4 3/3] rust: miscdevice: adjust the rust_misc_device sample to use RegistrationData.

[Christian Schrefl]

On 31.05.25 3:40 PM, Miguel Ojeda wrote:
> On Sat, May 31, 2025 at 2:27 PM Benno Lossin <lossin@kernel.org> wrote:
>>
>> Let's keep this newline.
> 
> There is also another apparently spurious change:
> 
>> -        pr_info!("Initialising Rust Misc Device Sample\n");
>> +        pr_info!("Initializing Rust Misc Device Sample\n");
> 
> In general, please try to avoid unrelated changes in patches.

Sorry, fixed all three for v5.

Cheers
Christian