[PATCH 0/7] misc device: support device drivers

[PATCH 0/7] misc device: support device drivers

[Danilo Krummrich]

This patch series adds support for device drivers to the misc device
abstraction.

For design details, please see:
  * patch 5 "rust: miscdevice: properly support device drivers"
  * patch 6 "rust: miscdevice: expose the parent device as &Device<Bound>"

This patch series depends on the pin-init series from Benno [1] as well as on
the misc device series from Christian [2], with UnsafePinned replaced with
Opaque, as suggested by Alice, since UnsafePinned may still take a while to
land.

A branch containing this series and its dependencies can be found in [3].

Thanks to Benno for his great help with pin-init!

[1] https://lore.kernel.org/rust-for-linux/20250529081027.297648-1-lossin@kernel.org/
[2] https://lore.kernel.org/lkml/20250517-b4-rust_miscdevice_registrationdata-v3-0-cdb33e228d37@gmail.com/
[3] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=rust/misc

Danilo Krummrich (7):
  rust: types: support fallible PinInit types in Opaque::pin_init
  rust: revocable: support fallible PinInit types
  rust: devres: support fallible in-place init for data
  rust: faux: impl AsRef<Device<Bound>> for Registration
  rust: miscdevice: properly support device drivers
  rust: miscdevice: expose the parent device as &Device<Bound>
  rust: sample: misc: implement device driver sample

 rust/kernel/devres.rs            |  27 ++-
 rust/kernel/faux.rs              |   4 +-
 rust/kernel/miscdevice.rs        | 341 ++++++++++++++++++++++---------
 rust/kernel/revocable.rs         |   7 +-
 rust/kernel/types.rs             |   8 +-
 samples/rust/Kconfig             |   8 +
 samples/rust/rust_misc_device.rs |  73 +++++--
 7 files changed, 350 insertions(+), 118 deletions(-)


base-commit: d5e34ea41dda1500a4dc163d6e96395fe7adc681
-- 
2.49.0

[PATCH 1/7] rust: types: support fallible PinInit types in Opaque::pin_init

[Danilo Krummrich]

Currently, Opaque::pin_init only supports infallible PinInit
implementations, i.e. impl PinInit<T, Infallible>.

This has been sufficient so far, since users such as Revocable do not
support fallibility.

Since this is about to change, make Opaque::pin_init() generic over the
error type E.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/types.rs | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 22985b6f6982..75c99d6facf9 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -354,13 +354,13 @@ pub const fn zeroed() -> 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| {
+    pub fn pin_init<E>(slot: impl PinInit<T, E>) -> impl PinInit<Self, E> {
+        Self::try_ffi_init(|ptr: *mut T| -> Result<(), E> {
             // SAFETY:
             //   - `ptr` is a valid pointer to uninitialized memory,
-            //   - `slot` is not accessed on error; the call is infallible,
+            //   - `slot` is not accessed on error,
             //   - `slot` is pinned in memory.
-            let _ = unsafe { PinInit::<T>::__pinned_init(slot, ptr) };
+            unsafe { PinInit::<T, E>::__pinned_init(slot, ptr) }
         })
     }
 
-- 
2.49.0

[PATCH 2/7] rust: revocable: support fallible PinInit types

[Danilo Krummrich]

Currently, Revocable::new() only supports infallible PinInit
implementations, i.e. impl PinInit<T, Infallible>.

This has been sufficient so far, since users such as Devres do not
support fallibility.

Since this is about to change, make Revocable::new() generic over the
error type E.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/devres.rs    | 2 +-
 rust/kernel/revocable.rs | 7 +++++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index 0f79a2ec9474..2dbe17d6ea1f 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -98,7 +98,7 @@ struct DevresInner<T> {
 impl<T> DevresInner<T> {
     fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
         let inner = Arc::pin_init(
-            pin_init!( DevresInner {
+            try_pin_init!( DevresInner {
                 dev: dev.into(),
                 callback: Self::devres_callback,
                 data <- Revocable::new(data),
diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
index db4aa46bb121..ca738f75dc10 100644
--- a/rust/kernel/revocable.rs
+++ b/rust/kernel/revocable.rs
@@ -80,8 +80,11 @@ unsafe impl<T: Sync + Send> Sync for Revocable<T> {}
 
 impl<T> Revocable<T> {
     /// Creates a new revocable instance of the given data.
-    pub fn new(data: impl PinInit<T>) -> impl PinInit<Self> {
-        pin_init!(Self {
+    pub fn new<E>(data: impl PinInit<T, E>) -> impl PinInit<Self, Error>
+    where
+        Error: From<E>,
+    {
+        try_pin_init!(Self {
             is_available: AtomicBool::new(true),
             data <- Opaque::pin_init(data),
         })
-- 
2.49.0

[PATCH 3/7] rust: devres: support fallible in-place init for data

[Danilo Krummrich]

Currently, Devres only supports a data argument of type T. However,
DevresInner already uses pin-init to initialize the Revocable. Hence,
there is no need for this limitation and we can take a data argument of
type impl PinInit<T, E> instead.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/devres.rs | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index 2dbe17d6ea1f..47aeb5196dd2 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -96,9 +96,16 @@ struct DevresInner<T> {
 pub struct Devres<T>(Arc<DevresInner<T>>);
 
 impl<T> DevresInner<T> {
-    fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
-        let inner = Arc::pin_init(
-            try_pin_init!( DevresInner {
+    fn new<E>(
+        dev: &Device<Bound>,
+        data: impl PinInit<T, E>,
+        flags: Flags,
+    ) -> Result<Arc<DevresInner<T>>>
+    where
+        Error: From<E>,
+    {
+        let inner = Arc::pin_init::<Error>(
+            try_pin_init!( Self {
                 dev: dev.into(),
                 callback: Self::devres_callback,
                 data <- Revocable::new(data),
@@ -168,7 +175,10 @@ fn remove_action(this: &Arc<Self>) {
 impl<T> Devres<T> {
     /// Creates a new [`Devres`] instance of the given `data`. The `data` encapsulated within the
     /// returned `Devres` instance' `data` will be revoked once the device is detached.
-    pub fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Self> {
+    pub fn new<E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flags) -> Result<Self>
+    where
+        Error: From<E>,
+    {
         let inner = DevresInner::new(dev, data, flags)?;
 
         Ok(Devres(inner))
@@ -176,7 +186,14 @@ pub fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Self> {
 
     /// Same as [`Devres::new`], but does not return a `Devres` instance. Instead the given `data`
     /// is owned by devres and will be revoked / dropped, once the device is detached.
-    pub fn new_foreign_owned(dev: &Device<Bound>, data: T, flags: Flags) -> Result {
+    pub fn new_foreign_owned<E>(
+        dev: &Device<Bound>,
+        data: impl PinInit<T, E>,
+        flags: Flags,
+    ) -> Result
+    where
+        Error: From<E>,
+    {
         let _ = DevresInner::new(dev, data, flags)?;
 
         Ok(())
-- 
2.49.0

[PATCH 4/7] rust: faux: impl AsRef<Device<Bound>> for Registration

[Danilo Krummrich]

The device contained in a faux::Registration is always bound, hence
consider this in the registrations's AsRef implementation.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/faux.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/faux.rs b/rust/kernel/faux.rs
index 8a50fcd4c9bb..395cb1172cb4 100644
--- a/rust/kernel/faux.rs
+++ b/rust/kernel/faux.rs
@@ -50,8 +50,8 @@ fn as_raw(&self) -> *mut bindings::faux_device {
     }
 }
 
-impl AsRef<device::Device> for Registration {
-    fn as_ref(&self) -> &device::Device {
+impl AsRef<device::Device<device::Bound>> for Registration {
+    fn as_ref(&self) -> &device::Device<device::Bound> {
         // SAFETY: The underlying `device` in `faux_device` is guaranteed by the C API to be
         // a valid initialized `device`.
         unsafe { device::Device::as_ref(addr_of_mut!((*self.as_raw()).dev)) }
-- 
2.49.0

[PATCH 5/7] rust: miscdevice: properly support device drivers

[Danilo Krummrich]

Currently, the design of MiscDeviceRegistration is focused on creating
and registering a misc device directly from a module and hence does not
support using misc device from a device driver.

However, it is important for the design of the misc device abstraction to
take the driver model into account.

Hence, consider the use-case of using misc device from a device driver;
let's go through the design motivation bottom-up:

Ideally, we want to be able to access (bus) device resources (such as I/O
memory) from misc device callbacks (such as open() or ioctl()) without
additional overhead, i.e. without having to go through
Devres::try_access(), which implies an atomic check and an RCU read side
critical section. Instead, we want to be able to use Devres::access(),
which does not have any overhead, which requires a &Device<Bound> to
prove that we can directly access the device resource.

Given that all misc device callbacks are synchronized against
misc_deregister(), we can prove that the misc device's parent device is
bound iff we guarantee that the misc device's registration won't
out-live the parent device's unbind.

This can easily be proven by using devres for the misc device's
registration object itself.

Since this is only applicable for the device driver use-case, abstract
the actual registration instance with a Rust enum, which is either a
"raw" registration or a "managed" registration.

In order to avoid any penalties from a managed registration, structurally
separate the registration's private data from the "raw" misc device
registration (which either stays "raw" or becomes "managed") depending
on whether a parent device is supplied.

The advantage of this solution is that it is entirely transparent to the
user -- no separate structures or functions for whether the abstraction
is used directly from a module or from a device driver; instead
MiscDeviceRegistration::register() gets an optional parent argument.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/miscdevice.rs        | 178 ++++++++++++++++++++++++-------
 samples/rust/rust_misc_device.rs |   9 +-
 2 files changed, 143 insertions(+), 44 deletions(-)

diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
index 1b5ec13868e2..6801fe72a8a6 100644
--- a/rust/kernel/miscdevice.rs
+++ b/rust/kernel/miscdevice.rs
@@ -10,16 +10,17 @@
 
 use crate::{
     bindings, container_of,
-    device::Device,
+    device::{Bound, Device},
+    devres::Devres,
     error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR},
     ffi::{c_int, c_long, c_uint, c_ulong},
     fs::File,
     prelude::*,
     seq_file::SeqFile,
     str::CStr,
-    types::{ForeignOwnable, Opaque},
+    types::{ARef, ForeignOwnable, Opaque},
 };
-use core::{marker::PhantomData, mem::MaybeUninit, pin::Pin};
+use core::{marker::PhantomData, mem::MaybeUninit, pin::Pin, ptr::NonNull};
 
 /// Options for creating a misc device.
 #[derive(Copy, Clone)]
@@ -40,44 +41,43 @@ pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
     }
 }
 
-/// A registration of a miscdevice.
-///
 /// # Invariants
 ///
-/// `inner` is a registered misc device.
+/// - `inner` is a registered misc device,
+/// - `data` is valid for the entire lifetime of `Self`.
 #[repr(C)]
 #[pin_data(PinnedDrop)]
-pub struct MiscDeviceRegistration<T: MiscDevice> {
+struct RawDeviceRegistration<T: MiscDevice> {
     #[pin]
     inner: Opaque<bindings::miscdevice>,
-    #[pin]
-    data: Opaque<T::RegistrationData>,
+    data: NonNull<T::RegistrationData>,
     _t: PhantomData<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(
+impl<T: MiscDevice> RawDeviceRegistration<T> {
+    fn new<'a>(
         opts: MiscDeviceOptions,
-        data: impl PinInit<T::RegistrationData, Error>,
-    ) -> impl PinInit<Self, Error> {
+        parent: Option<&'a Device<Bound>>,
+        data: &'a T::RegistrationData,
+    ) -> impl PinInit<Self, Error> + 'a
+    where
+        T: 'a,
+    {
         try_pin_init!(Self {
-            data <- Opaque::pin_init(data),
+            // INVARIANT: `Self` is always embedded in a `MiscDeviceRegistration<T>`, hence `data`
+            // is guaranteed to be valid for the entire lifetime of `Self`.
+            data: NonNull::from(data),
             inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
+                let mut value = opts.into_raw::<T>();
+
+                if let Some(parent) = parent {
+                    // The device core code will take care to take a reference of `parent` in
+                    // `device_add()` called by `misc_register()`.
+                    value.parent = parent.as_raw();
+                }
+
                 // SAFETY: The initializer can write to the provided `slot`.
-                unsafe { slot.write(opts.into_raw::<T>()) };
+                unsafe { slot.write(value) };
 
                 // SAFETY:
                 // * We just wrote the misc device options to the slot. The miscdevice will
@@ -94,12 +94,12 @@ pub fn register(
     }
 
     /// Returns a raw pointer to the misc device.
-    pub fn as_raw(&self) -> *mut bindings::miscdevice {
+    fn as_raw(&self) -> *mut bindings::miscdevice {
         self.inner.get()
     }
 
     /// Access the `this_device` field.
-    pub fn device(&self) -> &Device {
+    fn device(&self) -> &Device {
         // SAFETY: This can only be called after a successful register(), which always
         // initialises `this_device` with a valid device. Furthermore, the signature of this
         // function tells the borrow-checker that the `&Device` reference must not outlive the
@@ -108,6 +108,108 @@ pub fn device(&self) -> &Device {
         unsafe { Device::as_ref((*self.as_raw()).this_device) }
     }
 
+    fn data(&self) -> &T::RegistrationData {
+        // SAFETY: The type invariant guarantees that `data` is valid for the entire lifetime of
+        // `Self`.
+        unsafe { self.data.as_ref() }
+    }
+}
+
+#[pinned_drop]
+impl<T: MiscDevice> PinnedDrop for RawDeviceRegistration<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()) };
+    }
+}
+
+#[expect(dead_code)]
+enum DeviceRegistrationInner<T: MiscDevice> {
+    Raw(Pin<KBox<RawDeviceRegistration<T>>>),
+    Managed(Devres<RawDeviceRegistration<T>>),
+}
+
+/// A registration of a miscdevice.
+#[pin_data(PinnedDrop)]
+pub struct MiscDeviceRegistration<T: MiscDevice> {
+    inner: DeviceRegistrationInner<T>,
+    #[pin]
+    data: Opaque<T::RegistrationData>,
+    this_device: ARef<Device>,
+    _t: PhantomData<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<'a>(
+        opts: MiscDeviceOptions,
+        data: impl PinInit<T::RegistrationData, Error> + 'a,
+        parent: Option<&'a Device<Bound>>,
+    ) -> impl PinInit<Self, Error> + 'a
+    where
+        T: 'a,
+    {
+        let mut dev: Option<ARef<Device>> = None;
+
+        try_pin_init!(&this in Self {
+            data <- Opaque::pin_init(data),
+            // TODO: make `inner` in-place when enums get supported by pin-init.
+            //
+            // Link: https://github.com/Rust-for-Linux/pin-init/issues/59
+            inner: {
+                // SAFETY:
+                //   - `this` is a valid pointer to `Self`,
+                //   - `data` was properly initialized above.
+                let data = unsafe { &*(*this.as_ptr()).data.get() };
+
+                let raw = RawDeviceRegistration::new(opts, parent, data);
+
+                // FIXME: Work around a bug in rustc, to prevent the following warning:
+                //
+                //   "warning: value captured by `dev` is never read."
+                //
+                // Link: https://github.com/rust-lang/rust/issues/141615
+                let _ = dev;
+
+                if let Some(parent) = parent {
+                    let devres = Devres::new(parent, raw, GFP_KERNEL)?;
+
+                    dev = Some(devres.access(parent)?.device().into());
+                    DeviceRegistrationInner::Managed(devres)
+                } else {
+                    let boxed = KBox::pin_init(raw, GFP_KERNEL)?;
+
+                    dev = Some(boxed.device().into());
+                    DeviceRegistrationInner::Raw(boxed)
+                }
+            },
+            // Cache `this_device` within `Self` to avoid having to access `Devres` in the managed
+            // case.
+            this_device: {
+                // SAFETY: `dev` is guaranteed to be set in the initializer of `inner` above.
+                unsafe { dev.unwrap_unchecked() }
+            },
+            _t: PhantomData,
+        })
+    }
+
+    /// Access the `this_device` field.
+    pub fn device(&self) -> &Device {
+        &self.this_device
+    }
+
     /// Access the additional data stored in this registration.
     pub fn data(&self) -> &T::RegistrationData {
         // SAFETY:
@@ -120,9 +222,6 @@ pub fn data(&self) -> &T::RegistrationData {
 #[pinned_drop]
 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.
         unsafe { core::ptr::drop_in_place(self.data.get()) };
     }
@@ -137,14 +236,13 @@ pub trait MiscDevice: Sized {
     /// 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()`].
+    /// This data can be accessed in [`MiscDevice::open()`].
     type RegistrationData: Sync;
 
     /// Called when the misc device is opened.
     ///
     /// The returned pointer will be stored as the private data for the file.
-    fn open(_file: &File, _misc: &MiscDeviceRegistration<Self>) -> Result<Self::Ptr>;
+    fn open(_file: &File, _misc: &Device, _data: &Self::RegistrationData) -> Result<Self::Ptr>;
 
     /// Called when the misc device is released.
     fn release(device: Self::Ptr, _file: &File) {
@@ -217,17 +315,17 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
         // 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
+        // * The `misc_ptr` always points to the `inner` field of a `RawDeviceRegistration<T>`.
+        // * The `RawDeviceRegistration<T>` is valid until the `struct miscdevice` was
         //   unregistered.
-        let registration = unsafe { &*container_of!(misc_ptr, MiscDeviceRegistration<T>, inner) };
+        let registration = unsafe { &*container_of!(misc_ptr, RawDeviceRegistration<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, registration) {
+        let ptr = match T::open(file, registration.device(), registration.data()) {
             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 843442b0ea1d..b60fd063afa8 100644
--- a/samples/rust/rust_misc_device.rs
+++ b/samples/rust/rust_misc_device.rs
@@ -198,7 +198,8 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
         try_pin_init!(Self {
             _miscdev <- MiscDeviceRegistration::register(
                 options,
-                Arc::pin_init(new_mutex!(Inner { value: 0_i32 }), GFP_KERNEL)
+                Arc::pin_init(new_mutex!(Inner { value: 0_i32 }), GFP_KERNEL),
+                None,
             ),
         })
     }
@@ -222,15 +223,15 @@ impl MiscDevice for RustMiscDevice {
 
     type RegistrationData = Arc<Mutex<Inner>>;
 
-    fn open(_file: &File, misc: &MiscDeviceRegistration<Self>) -> Result<Pin<KBox<Self>>> {
-        let dev = ARef::from(misc.device());
+    fn open(_file: &File, misc: &Device, data: &Self::RegistrationData) -> Result<Pin<KBox<Self>>> {
+        let dev = ARef::from(misc);
 
         dev_info!(dev, "Opening Rust Misc Device Sample\n");
 
         KBox::try_pin_init(
             try_pin_init! {
                 RustMiscDevice {
-                    shared: misc.data().clone(),
+                    shared: data.clone(),
                     unique <- new_mutex!(Inner { value: 0_i32 }),
                     dev: dev,
                 }
-- 
2.49.0

[PATCH 6/7] rust: miscdevice: expose the parent device as &Device<Bound>

[Danilo Krummrich]

Now that the misc device abstraction design considers device drivers,
take advantage of the fact that we can safely expose the parent of the
misc device as &Device<Bound> within the misc device's file operations.

Drivers can use this bound device reference to access device resources
without additional overhead through Devres::access().

Expose the &Device<Bound> parent, the &Device representation of the misc
device and the registration data through a new type MiscArgs through the
MiscDevice callbacks.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/miscdevice.rs        | 173 ++++++++++++++++++++-----------
 samples/rust/rust_misc_device.rs |  20 ++--
 2 files changed, 128 insertions(+), 65 deletions(-)

diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
index 6801fe72a8a6..937d0945d827 100644
--- a/rust/kernel/miscdevice.rs
+++ b/rust/kernel/miscdevice.rs
@@ -18,7 +18,7 @@
     prelude::*,
     seq_file::SeqFile,
     str::CStr,
-    types::{ARef, ForeignOwnable, Opaque},
+    types::{ARef, Opaque},
 };
 use core::{marker::PhantomData, mem::MaybeUninit, pin::Pin, ptr::NonNull};
 
@@ -51,6 +51,7 @@ struct RawDeviceRegistration<T: MiscDevice> {
     #[pin]
     inner: Opaque<bindings::miscdevice>,
     data: NonNull<T::RegistrationData>,
+    private: Opaque<T::Ptr>,
     _t: PhantomData<T>,
 }
 
@@ -67,6 +68,7 @@ fn new<'a>(
             // INVARIANT: `Self` is always embedded in a `MiscDeviceRegistration<T>`, hence `data`
             // is guaranteed to be valid for the entire lifetime of `Self`.
             data: NonNull::from(data),
+            private: Opaque::uninit(),
             inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
                 let mut value = opts.into_raw::<T>();
 
@@ -227,11 +229,21 @@ fn drop(self: Pin<&mut Self>) {
     }
 }
 
+/// The arguments passed to the file operation callbacks of a [`MiscDeviceRegistration`].
+pub struct MiscArgs<'a, T: MiscDevice> {
+    /// The [`Device`] representation of the `struct miscdevice`.
+    pub device: &'a Device,
+    /// The parent [`Device`] of [`Self::device`].
+    pub parent: Option<&'a Device<Bound>>,
+    /// The `RegistrationData` passed to [`MiscDeviceRegistration::register`].
+    pub data: &'a T::RegistrationData,
+}
+
 /// Trait implemented by the private data of an open misc device.
 #[vtable]
 pub trait MiscDevice: Sized {
     /// What kind of pointer should `Self` be wrapped in.
-    type Ptr: ForeignOwnable + Send + Sync;
+    type Ptr: 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.
@@ -242,12 +254,7 @@ pub trait MiscDevice: Sized {
     /// Called when the misc device is opened.
     ///
     /// The returned pointer will be stored as the private data for the file.
-    fn open(_file: &File, _misc: &Device, _data: &Self::RegistrationData) -> Result<Self::Ptr>;
-
-    /// Called when the misc device is released.
-    fn release(device: Self::Ptr, _file: &File) {
-        drop(device);
-    }
+    fn open(_file: &File, _args: MiscArgs<'_, Self>) -> Result<Self::Ptr>;
 
     /// Handler for ioctls.
     ///
@@ -255,7 +262,8 @@ fn release(device: Self::Ptr, _file: &File) {
     ///
     /// [`kernel::ioctl`]: mod@crate::ioctl
     fn ioctl(
-        _device: <Self::Ptr as ForeignOwnable>::Borrowed<'_>,
+        _args: MiscArgs<'_, Self>,
+        _device: &Self::Ptr,
         _file: &File,
         _cmd: u32,
         _arg: usize,
@@ -272,7 +280,8 @@ fn ioctl(
     /// provided, then `compat_ptr_ioctl` will be used instead.
     #[cfg(CONFIG_COMPAT)]
     fn compat_ioctl(
-        _device: <Self::Ptr as ForeignOwnable>::Borrowed<'_>,
+        _args: MiscArgs<'_, Self>,
+        _device: &Self::Ptr,
         _file: &File,
         _cmd: u32,
         _arg: usize,
@@ -281,11 +290,7 @@ fn compat_ioctl(
     }
 
     /// Show info for this fd.
-    fn show_fdinfo(
-        _device: <Self::Ptr as ForeignOwnable>::Borrowed<'_>,
-        _m: &SeqFile,
-        _file: &File,
-    ) {
+    fn show_fdinfo(_args: MiscArgs<'_, Self>, _device: &Self::Ptr, _m: &SeqFile, _file: &File) {
         build_error!(VTABLE_DEFAULT_ERROR)
     }
 }
@@ -296,16 +301,15 @@ fn show_fdinfo(
 impl<T: MiscDevice> MiscdeviceVTable<T> {
     /// # Safety
     ///
-    /// `file` and `inode` must be the file and inode for a file that is undergoing initialization.
-    /// The file must be associated with a `MiscDeviceRegistration<T>`.
-    unsafe extern "C" fn open(inode: *mut bindings::inode, raw_file: *mut bindings::file) -> c_int {
-        // SAFETY: The pointers are valid and for a file being opened.
-        let ret = unsafe { bindings::generic_file_open(inode, raw_file) };
-        if ret != 0 {
-            return ret;
-        }
-
-        // SAFETY: The open call of a file can access the private data.
+    /// This function must only be called from misc device file operations with the `struct file`
+    /// pointer provided by the corresponding callback.
+    unsafe fn registration_from_file<'a>(
+        raw_file: *mut bindings::file,
+    ) -> &'a RawDeviceRegistration<T> {
+        // SAFETY:
+        // * Since `raw_file` comes from a misc device file operation callback, it is a pointer to a
+        //   valid `struct file`.
+        // * All file operations can access the file's private data.
         let misc_ptr = unsafe { (*raw_file).private_data };
 
         // This is a miscdevice, so `misc_open()` sets the private data to a pointer to the
@@ -313,30 +317,57 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
         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`.
+        // * File operation callbacks ensure that the `struct miscdevice` can't be unregistered and
+        //   freed during a call.
         // * The `misc_ptr` always points to the `inner` field of a `RawDeviceRegistration<T>`.
         // * The `RawDeviceRegistration<T>` is valid until the `struct miscdevice` was
         //   unregistered.
-        let registration = unsafe { &*container_of!(misc_ptr, RawDeviceRegistration<T>, inner) };
+        unsafe { &*container_of!(misc_ptr, RawDeviceRegistration<T>, inner) }
+    }
+
+    fn args_from_registration<'a>(registration: &'a RawDeviceRegistration<T>) -> MiscArgs<'a, T> {
+        let parent: Option<&'a Device<Bound>> = registration.device().parent().map(|parent| {
+            // SAFETY: We just convert from `&Device` into `Device<Bound>`.
+            unsafe { Device::as_ref(parent.as_raw()) }
+        });
+
+        MiscArgs {
+            device: registration.device(),
+            parent,
+            data: registration.data(),
+        }
+    }
+
+    /// # Safety
+    ///
+    /// `file` and `inode` must be the file and inode for a file that is undergoing initialization.
+    /// The file must be associated with a `MiscDeviceRegistration<T>`.
+    unsafe extern "C" fn open(inode: *mut bindings::inode, raw_file: *mut bindings::file) -> c_int {
+        // SAFETY: The pointers are valid and for a file being opened.
+        let ret = unsafe { bindings::generic_file_open(inode, raw_file) };
+        if ret != 0 {
+            return ret;
+        }
+
+        // SAFETY: Called from a misc device file operation callback with the corresponding pointer
+        // to a `struct file`.
+        let registration = unsafe { Self::registration_from_file(raw_file) };
 
         // 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, registration.device(), registration.data()) {
+        let ptr = match T::open(file, Self::args_from_registration(registration)) {
             Ok(ptr) => ptr,
             Err(err) => return err.to_errno(),
         };
 
-        // This overwrites the private data with the value specified by the user, changing the type
-        // of this file's private data. All future accesses to the private data is performed by
-        // other fops_* methods in this file, which all correctly cast the private data to the new
-        // type.
-        //
-        // SAFETY: The open call of a file can access the private data.
-        unsafe { (*raw_file).private_data = ptr.into_foreign().cast() };
+        // SAFETY:
+        // * We only ever write `registration.private` from `open()`, which does not race with other
+        //   file operation callbacks, i.e. there are no concurrent reads.
+        // * `registration.private.get()` is properly aligned.
+        unsafe { registration.private.get().write(ptr) };
 
         0
     }
@@ -346,15 +377,16 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
     /// `file` and `inode` must be the file and inode for a file that is being released. The file
     /// must be associated with a `MiscDeviceRegistration<T>`.
     unsafe extern "C" fn release(_inode: *mut bindings::inode, file: *mut bindings::file) -> c_int {
-        // SAFETY: The release call of a file owns the private data.
-        let private = unsafe { (*file).private_data }.cast();
-        // SAFETY: The release call of a file owns the private data.
-        let ptr = unsafe { <T::Ptr as ForeignOwnable>::from_foreign(private) };
+        // SAFETY: Called from a misc device file operation callback with the corresponding pointer
+        // to a `struct file`.
+        let registration = unsafe { Self::registration_from_file(file) };
 
         // SAFETY:
-        // * The file is valid for the duration of this call.
-        // * There is no active fdget_pos region on the file on this thread.
-        T::release(ptr, unsafe { File::from_raw_file(file) });
+        // * There won't be any subsequent reads or writes to `registration.private` once
+        //   `release()` is called.
+        // * `registration.private` has been initialized in `open()`.
+        // * `registration.private.get()` is properly aligned.
+        unsafe { core::ptr::drop_in_place(registration.private.get()) };
 
         0
     }
@@ -363,17 +395,25 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
     ///
     /// `file` must be a valid file that is associated with a `MiscDeviceRegistration<T>`.
     unsafe extern "C" fn ioctl(file: *mut bindings::file, cmd: c_uint, arg: c_ulong) -> c_long {
-        // SAFETY: The ioctl call of a file can access the private data.
-        let private = unsafe { (*file).private_data }.cast();
-        // SAFETY: Ioctl calls can borrow the private data of the file.
-        let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
+        // SAFETY: Called from a misc device file operation callback with the corresponding pointer
+        // to a `struct file`.
+        let registration = unsafe { Self::registration_from_file(file) };
+
+        // SAFETY:
+        // * `registration.private` is initialized in `open()`, which is guaranteed to called
+        //   before this callback.
+        // * `registration.private.get()` is properly aligned.
+        // * There are no concurrent writes.
+        let private = unsafe { &*registration.private.get() };
 
         // SAFETY:
         // * The file is valid for the duration of this call.
         // * There is no active fdget_pos region on the file on this thread.
         let file = unsafe { File::from_raw_file(file) };
 
-        match T::ioctl(device, file, cmd, arg) {
+        let args = Self::args_from_registration(registration);
+
+        match T::ioctl(args, private, file, cmd, arg) {
             Ok(ret) => ret as c_long,
             Err(err) => err.to_errno() as c_long,
         }
@@ -388,17 +428,25 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
         cmd: c_uint,
         arg: c_ulong,
     ) -> c_long {
-        // SAFETY: The compat ioctl call of a file can access the private data.
-        let private = unsafe { (*file).private_data }.cast();
-        // SAFETY: Ioctl calls can borrow the private data of the file.
-        let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
+        // SAFETY: Called from a misc device file operation callback with the corresponding pointer
+        // to a `struct file`.
+        let registration = unsafe { Self::registration_from_file(file) };
+
+        // SAFETY:
+        // * `registration.private` is initialized in `open()`, which is guaranteed to called
+        //   before this callback.
+        // * `registration.private.get()` is properly aligned.
+        // * There are no concurrent writes.
+        let private = unsafe { &*registration.private.get() };
 
         // SAFETY:
         // * The file is valid for the duration of this call.
         // * There is no active fdget_pos region on the file on this thread.
         let file = unsafe { File::from_raw_file(file) };
 
-        match T::compat_ioctl(device, file, cmd, arg) {
+        let args = Self::args_from_registration(registration);
+
+        match T::compat_ioctl(args, private, file, cmd, arg) {
             Ok(ret) => ret as c_long,
             Err(err) => err.to_errno() as c_long,
         }
@@ -409,10 +457,17 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
     /// - `file` must be a valid file that is associated with a `MiscDeviceRegistration<T>`.
     /// - `seq_file` must be a valid `struct seq_file` that we can write to.
     unsafe extern "C" fn show_fdinfo(seq_file: *mut bindings::seq_file, file: *mut bindings::file) {
-        // SAFETY: The release call of a file owns the private data.
-        let private = unsafe { (*file).private_data }.cast();
-        // SAFETY: Ioctl calls can borrow the private data of the file.
-        let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
+        // SAFETY: Called from a misc device file operation callback with the corresponding pointer
+        // to a `struct file`.
+        let registration = unsafe { Self::registration_from_file(file) };
+
+        // SAFETY:
+        // * `registration.private` is initialized in `open()`, which is guaranteed to called
+        //   before this callback.
+        // * `registration.private.get()` is properly aligned.
+        // * There are no concurrent writes.
+        let private = unsafe { &*registration.private.get() };
+
         // SAFETY:
         // * The file is valid for the duration of this call.
         // * There is no active fdget_pos region on the file on this thread.
@@ -421,7 +476,9 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
         // which this method is called.
         let m = unsafe { SeqFile::from_raw(seq_file) };
 
-        T::show_fdinfo(device, m, file);
+        let args = Self::args_from_registration(registration);
+
+        T::show_fdinfo(args, private, m, file);
     }
 
     const VTABLE: bindings::file_operations = bindings::file_operations {
diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
index b60fd063afa8..9bf1a0f64e6e 100644
--- a/samples/rust/rust_misc_device.rs
+++ b/samples/rust/rust_misc_device.rs
@@ -159,7 +159,7 @@
     device::Device,
     fs::File,
     ioctl::{_IO, _IOC_SIZE, _IOR, _IOW},
-    miscdevice::{MiscDevice, MiscDeviceOptions, MiscDeviceRegistration},
+    miscdevice::{MiscArgs, MiscDevice, MiscDeviceOptions, MiscDeviceRegistration},
     new_mutex,
     prelude::*,
     sync::{Arc, Mutex},
@@ -223,15 +223,15 @@ impl MiscDevice for RustMiscDevice {
 
     type RegistrationData = Arc<Mutex<Inner>>;
 
-    fn open(_file: &File, misc: &Device, data: &Self::RegistrationData) -> Result<Pin<KBox<Self>>> {
-        let dev = ARef::from(misc);
+    fn open(_file: &File, args: MiscArgs<'_, Self>) -> Result<Pin<KBox<Self>>> {
+        let dev = ARef::from(args.device);
 
         dev_info!(dev, "Opening Rust Misc Device Sample\n");
 
         KBox::try_pin_init(
             try_pin_init! {
                 RustMiscDevice {
-                    shared: data.clone(),
+                    shared: args.data.clone(),
                     unique <- new_mutex!(Inner { value: 0_i32 }),
                     dev: dev,
                 }
@@ -240,8 +240,14 @@ fn open(_file: &File, misc: &Device, data: &Self::RegistrationData) -> Result<Pi
         )
     }
 
-    fn ioctl(me: Pin<&RustMiscDevice>, _file: &File, cmd: u32, arg: usize) -> Result<isize> {
-        dev_info!(me.dev, "IOCTLing Rust Misc Device Sample\n");
+    fn ioctl(
+        args: MiscArgs<'_, Self>,
+        me: &Self::Ptr,
+        _file: &File,
+        cmd: u32,
+        arg: usize,
+    ) -> Result<isize> {
+        dev_info!(args.device, "IOCTLing Rust Misc Device Sample\n");
 
         let size = _IOC_SIZE(cmd);
 
@@ -256,7 +262,7 @@ fn ioctl(me: Pin<&RustMiscDevice>, _file: &File, cmd: u32, arg: usize) -> Result
             }
             RUST_MISC_DEV_HELLO => me.hello()?,
             _ => {
-                dev_err!(me.dev, "-> IOCTL not recognised: {}\n", cmd);
+                dev_err!(args.device, "-> IOCTL not recognised: {}\n", cmd);
                 return Err(ENOTTY);
             }
         };
-- 
2.49.0

[PATCH 7/7] rust: sample: misc: implement device driver sample

[Danilo Krummrich]

In order to demonstrate and test a MiscDeviceRegistration with a parent
device, introduce CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT.

If CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT=y the misc device sample
is initialized with a parent device (faux), otherwise it is initialized
without a parent device, i.e. the exact same way as without this patch.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 samples/rust/Kconfig             |  8 +++++
 samples/rust/rust_misc_device.rs | 50 +++++++++++++++++++++++++++++---
 2 files changed, 54 insertions(+), 4 deletions(-)

diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
index b1006ab4bc3c..9948ec0939ef 100644
--- a/samples/rust/Kconfig
+++ b/samples/rust/Kconfig
@@ -30,6 +30,14 @@ config SAMPLE_RUST_MISC_DEVICE
 
 	  If unsure, say N.
 
+config SAMPLE_RUST_MISC_DEVICE_WITH_PARENT
+	bool "Create a misc device with a parent device"
+	depends on SAMPLE_RUST_MISC_DEVICE
+	default n
+	help
+	  Say Y here if you want the misc device sample to create a misc
+	  device with a parent device.
+
 config SAMPLE_RUST_PRINT
 	tristate "Printing macros"
 	help
diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
index 9bf1a0f64e6e..175638d6d341 100644
--- a/samples/rust/rust_misc_device.rs
+++ b/samples/rust/rust_misc_device.rs
@@ -167,6 +167,9 @@
     uaccess::{UserSlice, UserSliceReader, UserSliceWriter},
 };
 
+#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
+use kernel::faux;
+
 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);
@@ -181,19 +184,33 @@
     license: "GPL",
 }
 
+#[cfg(not(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT))]
 #[pin_data]
 struct RustMiscDeviceModule {
     #[pin]
     _miscdev: MiscDeviceRegistration<RustMiscDevice>,
 }
 
-impl kernel::InPlaceModule for RustMiscDeviceModule {
-    fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
+#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
+struct RustMiscDeviceModule {
+    _faux: faux::Registration,
+    _miscdev: Pin<KBox<MiscDeviceRegistration<RustMiscDevice>>>,
+}
+
+impl RustMiscDeviceModule {
+    fn init() -> MiscDeviceOptions {
         pr_info!("Initializing Rust Misc Device Sample\n");
 
-        let options = MiscDeviceOptions {
+        MiscDeviceOptions {
             name: c_str!("rust-misc-device"),
-        };
+        }
+    }
+}
+
+#[cfg(not(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT))]
+impl kernel::InPlaceModule for RustMiscDeviceModule {
+    fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
+        let options = Self::init();
 
         try_pin_init!(Self {
             _miscdev <- MiscDeviceRegistration::register(
@@ -205,6 +222,31 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
     }
 }
 
+#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
+impl kernel::Module for RustMiscDeviceModule {
+    fn init(_module: &'static ThisModule) -> Result<Self> {
+        let options = Self::init();
+        let faux = faux::Registration::new(c_str!("rust-misc-device-sample"), None)?;
+
+        // For every other bus, this would be called from Driver::probe(), which would return a
+        // `Result<Pin<KBox<T>>>`, but faux always binds to a "dummy" driver, hence probe() is
+        // not required.
+        let misc = KBox::pin_init(
+            MiscDeviceRegistration::register(
+                options,
+                Arc::pin_init(new_mutex!(Inner { value: 0_i32 }), GFP_KERNEL),
+                Some(faux.as_ref()),
+            ),
+            GFP_KERNEL,
+        )?;
+
+        Ok(Self {
+            _faux: faux,
+            _miscdev: misc,
+        })
+    }
+}
+
 struct Inner {
     value: i32,
 }
-- 
2.49.0

Re: [PATCH 1/7] rust: types: support fallible PinInit types in Opaque::pin_init

[Christian Schrefl]

On 30.05.25 4:24 PM, Danilo Krummrich wrote:
> Currently, Opaque::pin_init only supports infallible PinInit
> implementations, i.e. impl PinInit<T, Infallible>.
> 
> This has been sufficient so far, since users such as Revocable do not
> support fallibility.
> 
> Since this is about to change, make Opaque::pin_init() generic over the
> error type E.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---

Reviewed-by: Christian Schrefl <chrisi.schrefl@gmail.com>

>  rust/kernel/types.rs | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 22985b6f6982..75c99d6facf9 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -354,13 +354,13 @@ pub const fn zeroed() -> 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| {
> +    pub fn pin_init<E>(slot: impl PinInit<T, E>) -> impl PinInit<Self, E> {
> +        Self::try_ffi_init(|ptr: *mut T| -> Result<(), E> {
>              // SAFETY:
>              //   - `ptr` is a valid pointer to uninitialized memory,
> -            //   - `slot` is not accessed on error; the call is infallible,
> +            //   - `slot` is not accessed on error,
>              //   - `slot` is pinned in memory.
> -            let _ = unsafe { PinInit::<T>::__pinned_init(slot, ptr) };
> +            unsafe { PinInit::<T, E>::__pinned_init(slot, ptr) }
>          })
>      }
>  

Re: [PATCH 2/7] rust: revocable: support fallible PinInit types

[Christian Schrefl]

On 30.05.25 4:24 PM, Danilo Krummrich wrote:
> Currently, Revocable::new() only supports infallible PinInit
> implementations, i.e. impl PinInit<T, Infallible>.
> 
> This has been sufficient so far, since users such as Devres do not
> support fallibility.
> 
> Since this is about to change, make Revocable::new() generic over the
> error type E.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---

Reviewed-by: Christian Schrefl <chrisi.schrefl@gmail.com>

>  rust/kernel/devres.rs    | 2 +-
>  rust/kernel/revocable.rs | 7 +++++--
>  2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> index 0f79a2ec9474..2dbe17d6ea1f 100644
> --- a/rust/kernel/devres.rs
> +++ b/rust/kernel/devres.rs
> @@ -98,7 +98,7 @@ struct DevresInner<T> {
>  impl<T> DevresInner<T> {
>      fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
>          let inner = Arc::pin_init(
> -            pin_init!( DevresInner {
> +            try_pin_init!( DevresInner {
>                  dev: dev.into(),
>                  callback: Self::devres_callback,
>                  data <- Revocable::new(data),
> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> index db4aa46bb121..ca738f75dc10 100644
> --- a/rust/kernel/revocable.rs
> +++ b/rust/kernel/revocable.rs
> @@ -80,8 +80,11 @@ unsafe impl<T: Sync + Send> Sync for Revocable<T> {}
>  
>  impl<T> Revocable<T> {
>      /// Creates a new revocable instance of the given data.
> -    pub fn new(data: impl PinInit<T>) -> impl PinInit<Self> {
> -        pin_init!(Self {
> +    pub fn new<E>(data: impl PinInit<T, E>) -> impl PinInit<Self, Error>
> +    where
> +        Error: From<E>,
> +    {
> +        try_pin_init!(Self {
>              is_available: AtomicBool::new(true),
>              data <- Opaque::pin_init(data),
>          })

Re: [PATCH 3/7] rust: devres: support fallible in-place init for data

[Christian Schrefl]

On 30.05.25 4:24 PM, Danilo Krummrich wrote:
> Currently, Devres only supports a data argument of type T. However,
> DevresInner already uses pin-init to initialize the Revocable. Hence,
> there is no need for this limitation and we can take a data argument of
> type impl PinInit<T, E> instead.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---

Reviewed-by: Christian Schrefl <chrisi.schrefl@gmail.com>

>  rust/kernel/devres.rs | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> index 2dbe17d6ea1f..47aeb5196dd2 100644
> --- a/rust/kernel/devres.rs
> +++ b/rust/kernel/devres.rs
> @@ -96,9 +96,16 @@ struct DevresInner<T> {
>  pub struct Devres<T>(Arc<DevresInner<T>>);
>  
>  impl<T> DevresInner<T> {
> -    fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
> -        let inner = Arc::pin_init(
> -            try_pin_init!( DevresInner {
> +    fn new<E>(
> +        dev: &Device<Bound>,
> +        data: impl PinInit<T, E>,
> +        flags: Flags,
> +    ) -> Result<Arc<DevresInner<T>>>
> +    where
> +        Error: From<E>,
> +    {
> +        let inner = Arc::pin_init::<Error>(
> +            try_pin_init!( Self {
>                  dev: dev.into(),
>                  callback: Self::devres_callback,
>                  data <- Revocable::new(data),
> @@ -168,7 +175,10 @@ fn remove_action(this: &Arc<Self>) {
>  impl<T> Devres<T> {
>      /// Creates a new [`Devres`] instance of the given `data`. The `data` encapsulated within the
>      /// returned `Devres` instance' `data` will be revoked once the device is detached.
> -    pub fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Self> {
> +    pub fn new<E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flags) -> Result<Self>
> +    where
> +        Error: From<E>,
> +    {
>          let inner = DevresInner::new(dev, data, flags)?;
>  
>          Ok(Devres(inner))
> @@ -176,7 +186,14 @@ pub fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Self> {
>  
>      /// Same as [`Devres::new`], but does not return a `Devres` instance. Instead the given `data`
>      /// is owned by devres and will be revoked / dropped, once the device is detached.
> -    pub fn new_foreign_owned(dev: &Device<Bound>, data: T, flags: Flags) -> Result {
> +    pub fn new_foreign_owned<E>(
> +        dev: &Device<Bound>,
> +        data: impl PinInit<T, E>,
> +        flags: Flags,
> +    ) -> Result
> +    where
> +        Error: From<E>,
> +    {
>          let _ = DevresInner::new(dev, data, flags)?;
>  
>          Ok(())

Re: [PATCH 0/7] misc device: support device drivers

[Christian Schrefl]

Hi Danilo

On 30.05.25 4:24 PM, Danilo Krummrich wrote:
> This patch series adds support for device drivers to the misc device
> abstraction.
> 
> For design details, please see:
>   * patch 5 "rust: miscdevice: properly support device drivers"
>   * patch 6 "rust: miscdevice: expose the parent device as &Device<Bound>"
> 
> This patch series depends on the pin-init series from Benno [1] as well as on
> the misc device series from Christian [2], with UnsafePinned replaced with
> Opaque, as suggested by Alice, since UnsafePinned may still take a while to
> land.

If you want I can send out a new version using `Opaque`.

We could also add a type alias like:

type UnsafePinned<T> = Opaque<T>;

That would then be removed in the `UnsafePinned` patches.
This should make the migration simpler.

Cheers
Christian

> 
> A branch containing this series and its dependencies can be found in [3].
> 
> Thanks to Benno for his great help with pin-init!
> 
> [1] https://lore.kernel.org/rust-for-linux/20250529081027.297648-1-lossin@kernel.org/
> [2] https://lore.kernel.org/lkml/20250517-b4-rust_miscdevice_registrationdata-v3-0-cdb33e228d37@gmail.com/
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=rust/misc
> 
> Danilo Krummrich (7):
>   rust: types: support fallible PinInit types in Opaque::pin_init
>   rust: revocable: support fallible PinInit types
>   rust: devres: support fallible in-place init for data
>   rust: faux: impl AsRef<Device<Bound>> for Registration
>   rust: miscdevice: properly support device drivers
>   rust: miscdevice: expose the parent device as &Device<Bound>
>   rust: sample: misc: implement device driver sample
> 
>  rust/kernel/devres.rs            |  27 ++-
>  rust/kernel/faux.rs              |   4 +-
>  rust/kernel/miscdevice.rs        | 341 ++++++++++++++++++++++---------
>  rust/kernel/revocable.rs         |   7 +-
>  rust/kernel/types.rs             |   8 +-
>  samples/rust/Kconfig             |   8 +
>  samples/rust/rust_misc_device.rs |  73 +++++--
>  7 files changed, 350 insertions(+), 118 deletions(-)
> 
> 
> base-commit: d5e34ea41dda1500a4dc163d6e96395fe7adc681

Re: [PATCH 0/7] misc device: support device drivers

[Christian Schrefl]

On 30.05.25 6:37 PM, Christian Schrefl wrote:
> Hi Danilo
> 
> On 30.05.25 4:24 PM, Danilo Krummrich wrote:
>> This patch series adds support for device drivers to the misc device
>> abstraction.
>>
>> For design details, please see:
>>   * patch 5 "rust: miscdevice: properly support device drivers"
>>   * patch 6 "rust: miscdevice: expose the parent device as &Device<Bound>"
>>
>> This patch series depends on the pin-init series from Benno [1] as well as on
>> the misc device series from Christian [2], with UnsafePinned replaced with
>> Opaque, as suggested by Alice, since UnsafePinned may still take a while to
>> land.
> 
> If you want I can send out a new version using `Opaque`.
> 
> We could also add a type alias like:
> 
> type UnsafePinned<T> = Opaque<T>;

I forgot that Opaque doesn't drop, this would not be quite as simple,
but with a newtype with a `Drop` impl it should be possible.

> 
> That would then be removed in the `UnsafePinned` patches.
> This should make the migration simpler.
> 
> Cheers
> Christian
> 
>>
>> A branch containing this series and its dependencies can be found in [3].
>>
>> Thanks to Benno for his great help with pin-init!
>>
>> [1] https://lore.kernel.org/rust-for-linux/20250529081027.297648-1-lossin@kernel.org/
>> [2] https://lore.kernel.org/lkml/20250517-b4-rust_miscdevice_registrationdata-v3-0-cdb33e228d37@gmail.com/
>> [3] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=rust/misc
>>
>> Danilo Krummrich (7):
>>   rust: types: support fallible PinInit types in Opaque::pin_init
>>   rust: revocable: support fallible PinInit types
>>   rust: devres: support fallible in-place init for data
>>   rust: faux: impl AsRef<Device<Bound>> for Registration
>>   rust: miscdevice: properly support device drivers
>>   rust: miscdevice: expose the parent device as &Device<Bound>
>>   rust: sample: misc: implement device driver sample
>>
>>  rust/kernel/devres.rs            |  27 ++-
>>  rust/kernel/faux.rs              |   4 +-
>>  rust/kernel/miscdevice.rs        | 341 ++++++++++++++++++++++---------
>>  rust/kernel/revocable.rs         |   7 +-
>>  rust/kernel/types.rs             |   8 +-
>>  samples/rust/Kconfig             |   8 +
>>  samples/rust/rust_misc_device.rs |  73 +++++--
>>  7 files changed, 350 insertions(+), 118 deletions(-)
>>
>>
>> base-commit: d5e34ea41dda1500a4dc163d6e96395fe7adc681
> 

Re: [PATCH 5/7] rust: miscdevice: properly support device drivers

[Christian Schrefl]

On 30.05.25 4:24 PM, Danilo Krummrich wrote:
> Currently, the design of MiscDeviceRegistration is focused on creating
> and registering a misc device directly from a module and hence does not
> support using misc device from a device driver.
> 
> However, it is important for the design of the misc device abstraction to
> take the driver model into account.
> 
> Hence, consider the use-case of using misc device from a device driver;
> let's go through the design motivation bottom-up:
> 
> Ideally, we want to be able to access (bus) device resources (such as I/O
> memory) from misc device callbacks (such as open() or ioctl()) without
> additional overhead, i.e. without having to go through
> Devres::try_access(), which implies an atomic check and an RCU read side
> critical section. Instead, we want to be able to use Devres::access(),
> which does not have any overhead, which requires a &Device<Bound> to
> prove that we can directly access the device resource.
> 
> Given that all misc device callbacks are synchronized against
> misc_deregister(), we can prove that the misc device's parent device is
> bound iff we guarantee that the misc device's registration won't
> out-live the parent device's unbind.
> 
> This can easily be proven by using devres for the misc device's
> registration object itself.
> 
> Since this is only applicable for the device driver use-case, abstract
> the actual registration instance with a Rust enum, which is either a
> "raw" registration or a "managed" registration.
> 
> In order to avoid any penalties from a managed registration, structurally
> separate the registration's private data from the "raw" misc device
> registration (which either stays "raw" or becomes "managed") depending
> on whether a parent device is supplied.
> 
> The advantage of this solution is that it is entirely transparent to the
> user -- no separate structures or functions for whether the abstraction
> is used directly from a module or from a device driver; instead
> MiscDeviceRegistration::register() gets an optional parent argument.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/miscdevice.rs        | 178 ++++++++++++++++++++++++-------
>  samples/rust/rust_misc_device.rs |   9 +-
>  2 files changed, 143 insertions(+), 44 deletions(-)
> 
> diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> index 1b5ec13868e2..6801fe72a8a6 100644
> --- a/rust/kernel/miscdevice.rs
> +++ b/rust/kernel/miscdevice.rs
> @@ -10,16 +10,17 @@
>  
>  use crate::{
>      bindings, container_of,
> -    device::Device,
> +    device::{Bound, Device},
> +    devres::Devres,
>      error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR},
>      ffi::{c_int, c_long, c_uint, c_ulong},
>      fs::File,
>      prelude::*,
>      seq_file::SeqFile,
>      str::CStr,
> -    types::{ForeignOwnable, Opaque},
> +    types::{ARef, ForeignOwnable, Opaque},
>  };
> -use core::{marker::PhantomData, mem::MaybeUninit, pin::Pin};
> +use core::{marker::PhantomData, mem::MaybeUninit, pin::Pin, ptr::NonNull};
>  
>  /// Options for creating a misc device.
>  #[derive(Copy, Clone)]
> @@ -40,44 +41,43 @@ pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
>      }
>  }
>  
> -/// A registration of a miscdevice.
> -///
>  /// # Invariants
>  ///
> -/// `inner` is a registered misc device.
> +/// - `inner` is a registered misc device,
> +/// - `data` is valid for the entire lifetime of `Self`.
>  #[repr(C)]
>  #[pin_data(PinnedDrop)]
> -pub struct MiscDeviceRegistration<T: MiscDevice> {
> +struct RawDeviceRegistration<T: MiscDevice> {
>      #[pin]
>      inner: Opaque<bindings::miscdevice>,
> -    #[pin]
> -    data: Opaque<T::RegistrationData>,
> +    data: NonNull<T::RegistrationData>,
>      _t: PhantomData<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(
> +impl<T: MiscDevice> RawDeviceRegistration<T> {
> +    fn new<'a>(
>          opts: MiscDeviceOptions,
> -        data: impl PinInit<T::RegistrationData, Error>,
> -    ) -> impl PinInit<Self, Error> {
> +        parent: Option<&'a Device<Bound>>,
> +        data: &'a T::RegistrationData,
> +    ) -> impl PinInit<Self, Error> + 'a
> +    where
> +        T: 'a,
> +    {
>          try_pin_init!(Self {
> -            data <- Opaque::pin_init(data),
> +            // INVARIANT: `Self` is always embedded in a `MiscDeviceRegistration<T>`, hence `data`
> +            // is guaranteed to be valid for the entire lifetime of `Self`.
> +            data: NonNull::from(data),
>              inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
> +                let mut value = opts.into_raw::<T>();
> +
> +                if let Some(parent) = parent {
> +                    // The device core code will take care to take a reference of `parent` in
> +                    // `device_add()` called by `misc_register()`.
> +                    value.parent = parent.as_raw();
> +                }
> +
>                  // SAFETY: The initializer can write to the provided `slot`.
> -                unsafe { slot.write(opts.into_raw::<T>()) };
> +                unsafe { slot.write(value) };
>  
>                  // SAFETY:
>                  // * We just wrote the misc device options to the slot. The miscdevice will
> @@ -94,12 +94,12 @@ pub fn register(
>      }
>  
>      /// Returns a raw pointer to the misc device.
> -    pub fn as_raw(&self) -> *mut bindings::miscdevice {
> +    fn as_raw(&self) -> *mut bindings::miscdevice {
>          self.inner.get()
>      }
>  
>      /// Access the `this_device` field.
> -    pub fn device(&self) -> &Device {
> +    fn device(&self) -> &Device {
>          // SAFETY: This can only be called after a successful register(), which always
>          // initialises `this_device` with a valid device. Furthermore, the signature of this
>          // function tells the borrow-checker that the `&Device` reference must not outlive the
> @@ -108,6 +108,108 @@ pub fn device(&self) -> &Device {
>          unsafe { Device::as_ref((*self.as_raw()).this_device) }
>      }
>  
> +    fn data(&self) -> &T::RegistrationData {
> +        // SAFETY: The type invariant guarantees that `data` is valid for the entire lifetime of
> +        // `Self`.
> +        unsafe { self.data.as_ref() }
> +    }
> +}
> +
> +#[pinned_drop]
> +impl<T: MiscDevice> PinnedDrop for RawDeviceRegistration<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()) };
> +    }> +}
> +
> +#[expect(dead_code)]
> +enum DeviceRegistrationInner<T: MiscDevice> {
> +    Raw(Pin<KBox<RawDeviceRegistration<T>>>),
> +    Managed(Devres<RawDeviceRegistration<T>>),
> +}
> +
> +/// A registration of a miscdevice.
> +#[pin_data(PinnedDrop)]
> +pub struct MiscDeviceRegistration<T: MiscDevice> {
> +    inner: DeviceRegistrationInner<T>,
> +    #[pin]
> +    data: Opaque<T::RegistrationData>,
> +    this_device: ARef<Device>,
> +    _t: PhantomData<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<'a>(
> +        opts: MiscDeviceOptions,
> +        data: impl PinInit<T::RegistrationData, Error> + 'a,
> +        parent: Option<&'a Device<Bound>>,
> +    ) -> impl PinInit<Self, Error> + 'a
> +    where
> +        T: 'a,
> +    {
> +        let mut dev: Option<ARef<Device>> = None;
> +
> +        try_pin_init!(&this in Self {
> +            data <- Opaque::pin_init(data),
> +            // TODO: make `inner` in-place when enums get supported by pin-init.
> +            //
> +            // Link: https://github.com/Rust-for-Linux/pin-init/issues/59
> +            inner: {
> +                // SAFETY:
> +                //   - `this` is a valid pointer to `Self`,
> +                //   - `data` was properly initialized above.
> +                let data = unsafe { &*(*this.as_ptr()).data.get() };
> +
> +                let raw = RawDeviceRegistration::new(opts, parent, data);
> +
> +                // FIXME: Work around a bug in rustc, to prevent the following warning:
> +                //
> +                //   "warning: value captured by `dev` is never read."
> +                //
> +                // Link: https://github.com/rust-lang/rust/issues/141615
> +                let _ = dev;
> +
> +                if let Some(parent) = parent {
> +                    let devres = Devres::new(parent, raw, GFP_KERNEL)?;
> +
> +                    dev = Some(devres.access(parent)?.device().into());
> +                    DeviceRegistrationInner::Managed(devres)
> +                } else {
> +                    let boxed = KBox::pin_init(raw, GFP_KERNEL)?;
> +
> +                    dev = Some(boxed.device().into());
> +                    DeviceRegistrationInner::Raw(boxed)
> +                }
> +            },
> +            // Cache `this_device` within `Self` to avoid having to access `Devres` in the managed
> +            // case.
> +            this_device: {
> +                // SAFETY: `dev` is guaranteed to be set in the initializer of `inner` above.
> +                unsafe { dev.unwrap_unchecked() }
> +            },
> +            _t: PhantomData,
> +        })
> +    }
> +
> +    /// Access the `this_device` field.
> +    pub fn device(&self) -> &Device {
> +        &self.this_device
> +    }
> +
>      /// Access the additional data stored in this registration.
>      pub fn data(&self) -> &T::RegistrationData {
>          // SAFETY:
> @@ -120,9 +222,6 @@ pub fn data(&self) -> &T::RegistrationData {
>  #[pinned_drop]
>  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.
>          unsafe { core::ptr::drop_in_place(self.data.get()) };

I think this can race for a use after free.
The data gets freed in this `Drop` impl but the `miscdevice_deregister` call will only
happen in the `DeviceRegistrationInner` `Drop` implementatation, since the fields 
will only be dropped after the `drop` function has executed.

Either  inner: DeviceRegistrationInner<T> needs to be wrapped in a ManuallyDrop and
dropped manually,
or the Data needs to be wrapped in a type that will automatically drop it (this would
be fine with `UnsafePinned`).


>      }
> @@ -137,14 +236,13 @@ pub trait MiscDevice: Sized {
>      /// 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()`].
> +    /// This data can be accessed in [`MiscDevice::open()`].
>      type RegistrationData: Sync;
>  
>      /// Called when the misc device is opened.
>      ///
>      /// The returned pointer will be stored as the private data for the file.
> -    fn open(_file: &File, _misc: &MiscDeviceRegistration<Self>) -> Result<Self::Ptr>;
> +    fn open(_file: &File, _misc: &Device, _data: &Self::RegistrationData) -> Result<Self::Ptr>;
>  
>      /// Called when the misc device is released.
>      fn release(device: Self::Ptr, _file: &File) {
> @@ -217,17 +315,17 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
>          // 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
> +        // * The `misc_ptr` always points to the `inner` field of a `RawDeviceRegistration<T>`.
> +        // * The `RawDeviceRegistration<T>` is valid until the `struct miscdevice` was
>          //   unregistered.
> -        let registration = unsafe { &*container_of!(misc_ptr, MiscDeviceRegistration<T>, inner) };
> +        let registration = unsafe { &*container_of!(misc_ptr, RawDeviceRegistration<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, registration) {
> +        let ptr = match T::open(file, registration.device(), registration.data()) {
>              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 843442b0ea1d..b60fd063afa8 100644
> --- a/samples/rust/rust_misc_device.rs
> +++ b/samples/rust/rust_misc_device.rs
> @@ -198,7 +198,8 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
>          try_pin_init!(Self {
>              _miscdev <- MiscDeviceRegistration::register(
>                  options,
> -                Arc::pin_init(new_mutex!(Inner { value: 0_i32 }), GFP_KERNEL)
> +                Arc::pin_init(new_mutex!(Inner { value: 0_i32 }), GFP_KERNEL),
> +                None,
>              ),
>          })
>      }
> @@ -222,15 +223,15 @@ impl MiscDevice for RustMiscDevice {
>  
>      type RegistrationData = Arc<Mutex<Inner>>;
>  
> -    fn open(_file: &File, misc: &MiscDeviceRegistration<Self>) -> Result<Pin<KBox<Self>>> {
> -        let dev = ARef::from(misc.device());
> +    fn open(_file: &File, misc: &Device, data: &Self::RegistrationData) -> Result<Pin<KBox<Self>>> {
> +        let dev = ARef::from(misc);
>  
>          dev_info!(dev, "Opening Rust Misc Device Sample\n");
>  
>          KBox::try_pin_init(
>              try_pin_init! {
>                  RustMiscDevice {
> -                    shared: misc.data().clone(),
> +                    shared: data.clone(),
>                      unique <- new_mutex!(Inner { value: 0_i32 }),
>                      dev: dev,
>                  }

Re: [PATCH 5/7] rust: miscdevice: properly support device drivers

[Danilo Krummrich]

On Fri, May 30, 2025 at 07:35:56PM +0200, Christian Schrefl wrote:
> On 30.05.25 4:24 PM, Danilo Krummrich wrote:

<snip>

> > +#[pinned_drop]
> > +impl<T: MiscDevice> PinnedDrop for RawDeviceRegistration<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()) };
> > +    }> +}

<snip>

> >  #[pinned_drop]
> >  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.
> >          unsafe { core::ptr::drop_in_place(self.data.get()) };
> 
> I think this can race for a use after free.
> The data gets freed in this `Drop` impl but the `miscdevice_deregister` call will only
> happen in the `DeviceRegistrationInner` `Drop` implementatation, since the fields 
> will only be dropped after the `drop` function has executed.

Yes and no. :-) The fun part is that the drop order actually depends on whether
we have a parent device and use Devres or whether we have no parent device.

In the first case the drop order is correct by chance, due to Devres revoking
things when the parent device in unbound, which happens before, since the faux
device is dropped first.

But in general you're right, this needs to be fixed.

> Either  inner: DeviceRegistrationInner<T> needs to be wrapped in a ManuallyDrop and
> dropped manually,
> or the Data needs to be wrapped in a type that will automatically drop it (this would
> be fine with `UnsafePinned`).

There's also option 3), which is moving the drop_in_place() for the registration
data into RawDeviceRegistration::drop(), right below misc_deregister(), until we
have `UnsafePinned`.

(This is fine, since RawDeviceRegistration already has (and requires) a pointer
to the registration data and hence is always embedded in a
MiscDeviceRegistration.)

Re: [PATCH 0/7] misc device: support device drivers

[Danilo Krummrich]

Hi Christian,

On Fri, May 30, 2025 at 06:37:29PM +0200, Christian Schrefl wrote:
> If you want I can send out a new version using `Opaque`.

Sounds good, you can also give me a version that you want me to add to this
series, given that in order to use Opaque you also need the Opaque patch of this
series. Without you can't pass an impl PinInit<T, Error> to Opaque.

Re: [PATCH 0/7] misc device: support device drivers

[Benno Lossin]

On Fri May 30, 2025 at 7:29 PM CEST, Christian Schrefl wrote:
> On 30.05.25 6:37 PM, Christian Schrefl wrote:
>> On 30.05.25 4:24 PM, Danilo Krummrich wrote:
>>> This patch series adds support for device drivers to the misc device
>>> abstraction.
>>>
>>> For design details, please see:
>>>   * patch 5 "rust: miscdevice: properly support device drivers"
>>>   * patch 6 "rust: miscdevice: expose the parent device as &Device<Bound>"
>>>
>>> This patch series depends on the pin-init series from Benno [1] as well as on
>>> the misc device series from Christian [2], with UnsafePinned replaced with
>>> Opaque, as suggested by Alice, since UnsafePinned may still take a while to
>>> land.
>> 
>> If you want I can send out a new version using `Opaque`.
>> 
>> We could also add a type alias like:
>> 
>> type UnsafePinned<T> = Opaque<T>;
>
> I forgot that Opaque doesn't drop, this would not be quite as simple,
> but with a newtype with a `Drop` impl it should be possible.

That's one issue another is that `Opaque` also allows uninitialized
memory and (if the upstream one isn't changed) also modifies the
behavior of shared references. I don't think we should name it
`UnsafePinned` if it doesn't guarantee the same thing as the upstream
one.

---
Cheers,
Benno

Re: [PATCH 0/7] misc device: support device drivers

[Benno Lossin]

On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
> This patch series adds support for device drivers to the misc device
> abstraction.
>
> For design details, please see:
>   * patch 5 "rust: miscdevice: properly support device drivers"
>   * patch 6 "rust: miscdevice: expose the parent device as &Device<Bound>"
>
> This patch series depends on the pin-init series from Benno [1] as well as on
> the misc device series from Christian [2], with UnsafePinned replaced with
> Opaque, as suggested by Alice, since UnsafePinned may still take a while to
> land.
>
> A branch containing this series and its dependencies can be found in [3].
>
> Thanks to Benno for his great help with pin-init!

No problem! I also found it very useful to get some more uses for
pin-init. This way future contributors will benefit from the new
features :)

---
Cheers,
Benno

Re: [PATCH 1/7] rust: types: support fallible PinInit types in Opaque::pin_init

[Benno Lossin]

On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
> Currently, Opaque::pin_init only supports infallible PinInit
> implementations, i.e. impl PinInit<T, Infallible>.
>
> This has been sufficient so far, since users such as Revocable do not
> support fallibility.
>
> Since this is about to change, make Opaque::pin_init() generic over the
> error type E.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/types.rs | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 22985b6f6982..75c99d6facf9 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -354,13 +354,13 @@ pub const fn zeroed() -> 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| {
> +    pub fn pin_init<E>(slot: impl PinInit<T, E>) -> impl PinInit<Self, E> {
> +        Self::try_ffi_init(|ptr: *mut T| -> Result<(), E> {
>              // SAFETY:
>              //   - `ptr` is a valid pointer to uninitialized memory,
> -            //   - `slot` is not accessed on error; the call is infallible,
> +            //   - `slot` is not accessed on error,
>              //   - `slot` is pinned in memory.
> -            let _ = unsafe { PinInit::<T>::__pinned_init(slot, ptr) };
> +            unsafe { PinInit::<T, E>::__pinned_init(slot, ptr) }

Could you move this function into an `impl pin_init::Wrapper<T>` block?
(it's the same function, but in a trait that was recently added)

Thanks!

---
Cheers,
Benno

>          })
>      }
>  

Re: [PATCH 2/7] rust: revocable: support fallible PinInit types

[Benno Lossin]

On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
> Currently, Revocable::new() only supports infallible PinInit
> implementations, i.e. impl PinInit<T, Infallible>.
>
> This has been sufficient so far, since users such as Devres do not
> support fallibility.
>
> Since this is about to change, make Revocable::new() generic over the
> error type E.

You could add some information about the design decisions, ie choosing
`Error` & asking for `Error: From<E>` vs just using `E`.

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

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

---
Cheers,
Benno

> ---
>  rust/kernel/devres.rs    | 2 +-
>  rust/kernel/revocable.rs | 7 +++++--
>  2 files changed, 6 insertions(+), 3 deletions(-)

Re: [PATCH 3/7] rust: devres: support fallible in-place init for data

[Benno Lossin]

On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
> Currently, Devres only supports a data argument of type T. However,
> DevresInner already uses pin-init to initialize the Revocable. Hence,
> there is no need for this limitation and we can take a data argument of
> type impl PinInit<T, E> instead.

Missing '`'.

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

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

> ---
>  rust/kernel/devres.rs | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> index 2dbe17d6ea1f..47aeb5196dd2 100644
> --- a/rust/kernel/devres.rs
> +++ b/rust/kernel/devres.rs
> @@ -96,9 +96,16 @@ struct DevresInner<T> {
>  pub struct Devres<T>(Arc<DevresInner<T>>);
>  
>  impl<T> DevresInner<T> {
> -    fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
> -        let inner = Arc::pin_init(
> -            try_pin_init!( DevresInner {
> +    fn new<E>(
> +        dev: &Device<Bound>,
> +        data: impl PinInit<T, E>,
> +        flags: Flags,
> +    ) -> Result<Arc<DevresInner<T>>>
> +    where
> +        Error: From<E>,
> +    {
> +        let inner = Arc::pin_init::<Error>(
> +            try_pin_init!( Self {

Spurious space between `!(` and `Self {`.

---
Cheers,
Benno

Re: [PATCH 0/7] misc device: support device drivers

[Boqun Feng]

On Fri, May 30, 2025 at 09:24:28PM +0200, Benno Lossin wrote:
> On Fri May 30, 2025 at 7:29 PM CEST, Christian Schrefl wrote:
> > On 30.05.25 6:37 PM, Christian Schrefl wrote:
> >> On 30.05.25 4:24 PM, Danilo Krummrich wrote:
> >>> This patch series adds support for device drivers to the misc device
> >>> abstraction.
> >>>
> >>> For design details, please see:
> >>>   * patch 5 "rust: miscdevice: properly support device drivers"
> >>>   * patch 6 "rust: miscdevice: expose the parent device as &Device<Bound>"
> >>>
> >>> This patch series depends on the pin-init series from Benno [1] as well as on
> >>> the misc device series from Christian [2], with UnsafePinned replaced with
> >>> Opaque, as suggested by Alice, since UnsafePinned may still take a while to
> >>> land.

Maybe I'm missing something, but don't we have our own version of
`UnsafePinned` [1] which can be replaced once Rust upstream has the
`UnsafePinned` stabilized. I don't see any discussion about abandoning
that effort.

Regards,
Boqun

> >> 
> >> If you want I can send out a new version using `Opaque`.
> >> 
> >> We could also add a type alias like:
> >> 
> >> type UnsafePinned<T> = Opaque<T>;
> >
> > I forgot that Opaque doesn't drop, this would not be quite as simple,
> > but with a newtype with a `Drop` impl it should be possible.
> 
> That's one issue another is that `Opaque` also allows uninitialized
> memory and (if the upstream one isn't changed) also modifies the
> behavior of shared references. I don't think we should name it
> `UnsafePinned` if it doesn't guarantee the same thing as the upstream
> one.
> 
> ---
> Cheers,
> Benno

Re: [PATCH 0/7] misc device: support device drivers

[Boqun Feng]

On Fri, May 30, 2025 at 12:35:21PM -0700, Boqun Feng wrote:
> On Fri, May 30, 2025 at 09:24:28PM +0200, Benno Lossin wrote:
> > On Fri May 30, 2025 at 7:29 PM CEST, Christian Schrefl wrote:
> > > On 30.05.25 6:37 PM, Christian Schrefl wrote:
> > >> On 30.05.25 4:24 PM, Danilo Krummrich wrote:
> > >>> This patch series adds support for device drivers to the misc device
> > >>> abstraction.
> > >>>
> > >>> For design details, please see:
> > >>>   * patch 5 "rust: miscdevice: properly support device drivers"
> > >>>   * patch 6 "rust: miscdevice: expose the parent device as &Device<Bound>"
> > >>>
> > >>> This patch series depends on the pin-init series from Benno [1] as well as on
> > >>> the misc device series from Christian [2], with UnsafePinned replaced with
> > >>> Opaque, as suggested by Alice, since UnsafePinned may still take a while to
> > >>> land.
> 
> Maybe I'm missing something, but don't we have our own version of
> `UnsafePinned` [1] which can be replaced once Rust upstream has the
> `UnsafePinned` stabilized. I don't see any discussion about abandoning
> that effort.
> 

Missing the link..

[1]: https://lore.kernel.org/rust-for-linux/20250430-rust_unsafe_pinned-v2-0-fc8617a74024@gmail.com/

> Regards,
> Boqun
> 
> > >> 
> > >> If you want I can send out a new version using `Opaque`.
> > >> 
> > >> We could also add a type alias like:
> > >> 
> > >> type UnsafePinned<T> = Opaque<T>;
> > >
> > > I forgot that Opaque doesn't drop, this would not be quite as simple,
> > > but with a newtype with a `Drop` impl it should be possible.
> > 
> > That's one issue another is that `Opaque` also allows uninitialized
> > memory and (if the upstream one isn't changed) also modifies the
> > behavior of shared references. I don't think we should name it
> > `UnsafePinned` if it doesn't guarantee the same thing as the upstream
> > one.
> > 
> > ---
> > Cheers,
> > Benno

Re: [PATCH 5/7] rust: miscdevice: properly support device drivers

[Benno Lossin]

On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
> @@ -40,44 +41,43 @@ pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
>      }
>  }
>  
> -/// A registration of a miscdevice.
> -///
>  /// # Invariants
>  ///
> -/// `inner` is a registered misc device.
> +/// - `inner` is a registered misc device,
> +/// - `data` is valid for the entire lifetime of `Self`.
>  #[repr(C)]
>  #[pin_data(PinnedDrop)]
> -pub struct MiscDeviceRegistration<T: MiscDevice> {
> +struct RawDeviceRegistration<T: MiscDevice> {
>      #[pin]
>      inner: Opaque<bindings::miscdevice>,
> -    #[pin]
> -    data: Opaque<T::RegistrationData>,
> +    data: NonNull<T::RegistrationData>,
>      _t: PhantomData<T>,

You shouldn't need the `PhantomData` here.

Also, do we need to ask for `T: MiscDevice` here? Could we instead have
just `T` and then below you write
`RawDeviceRegistration<T::RegistrationData>` instead? (`new` of course
needs to have a new generic: `U: MiscDevice<RegistrationData = 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(
> +impl<T: MiscDevice> RawDeviceRegistration<T> {
> +    fn new<'a>(
>          opts: MiscDeviceOptions,
> -        data: impl PinInit<T::RegistrationData, Error>,
> -    ) -> impl PinInit<Self, Error> {
> +        parent: Option<&'a Device<Bound>>,
> +        data: &'a T::RegistrationData,
> +    ) -> impl PinInit<Self, Error> + 'a
> +    where
> +        T: 'a,
> +    {
>          try_pin_init!(Self {
> -            data <- Opaque::pin_init(data),
> +            // INVARIANT: `Self` is always embedded in a `MiscDeviceRegistration<T>`, hence `data`
> +            // is guaranteed to be valid for the entire lifetime of `Self`.
> +            data: NonNull::from(data),

Both the argument in the INVARIANT comment and way this works are a bit
flawed. Instead, I'd recommend directly taking the `NonNull` as a
parameter. Yes the function will need to be `unsafe`, but the lifetime
that you're creating below only lives for `'a`, but the object might
live much longer. You might still be fine, but I'd just recommend
staying in raw pointer land (or in this case `NonNull`).

>              inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
> +                let mut value = opts.into_raw::<T>();
> +
> +                if let Some(parent) = parent {
> +                    // The device core code will take care to take a reference of `parent` in

Just a question: with "take a reference of" you mean that it will
increment the refcount?

> +                    // `device_add()` called by `misc_register()`.
> +                    value.parent = parent.as_raw();
> +                }
> +
>                  // SAFETY: The initializer can write to the provided `slot`.
> -                unsafe { slot.write(opts.into_raw::<T>()) };
> +                unsafe { slot.write(value) };
>  
>                  // SAFETY:
>                  // * We just wrote the misc device options to the slot. The miscdevice will
> @@ -94,12 +94,12 @@ pub fn register(
>      }
>  
>      /// Returns a raw pointer to the misc device.
> -    pub fn as_raw(&self) -> *mut bindings::miscdevice {
> +    fn as_raw(&self) -> *mut bindings::miscdevice {
>          self.inner.get()
>      }
>  
>      /// Access the `this_device` field.
> -    pub fn device(&self) -> &Device {
> +    fn device(&self) -> &Device {
>          // SAFETY: This can only be called after a successful register(), which always
>          // initialises `this_device` with a valid device. Furthermore, the signature of this
>          // function tells the borrow-checker that the `&Device` reference must not outlive the
> @@ -108,6 +108,108 @@ pub fn device(&self) -> &Device {
>          unsafe { Device::as_ref((*self.as_raw()).this_device) }
>      }
>  
> +    fn data(&self) -> &T::RegistrationData {
> +        // SAFETY: The type invariant guarantees that `data` is valid for the entire lifetime of
> +        // `Self`.
> +        unsafe { self.data.as_ref() }
> +    }
> +}
> +
> +#[pinned_drop]
> +impl<T: MiscDevice> PinnedDrop for RawDeviceRegistration<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()) };
> +    }
> +}
> +
> +#[expect(dead_code)]
> +enum DeviceRegistrationInner<T: MiscDevice> {
> +    Raw(Pin<KBox<RawDeviceRegistration<T>>>),
> +    Managed(Devres<RawDeviceRegistration<T>>),

These two names could be shortened (`DeviceRegistrationInner` and
`RawDeviceRegistration`) as they are only implementation details of this
file. How about `InnerRegistration` and `RawRegistration`? Or maybe
something even shorter.

> +}
> +
> +/// A registration of a miscdevice.
> +#[pin_data(PinnedDrop)]
> +pub struct MiscDeviceRegistration<T: MiscDevice> {
> +    inner: DeviceRegistrationInner<T>,
> +    #[pin]
> +    data: Opaque<T::RegistrationData>,

Why is it necessary to store `data` inside of `Opaque`?

> +    this_device: ARef<Device>,
> +    _t: PhantomData<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<'a>(
> +        opts: MiscDeviceOptions,
> +        data: impl PinInit<T::RegistrationData, Error> + 'a,
> +        parent: Option<&'a Device<Bound>>,
> +    ) -> impl PinInit<Self, Error> + 'a
> +    where
> +        T: 'a,
> +    {
> +        let mut dev: Option<ARef<Device>> = None;
> +
> +        try_pin_init!(&this in Self {
> +            data <- Opaque::pin_init(data),
> +            // TODO: make `inner` in-place when enums get supported by pin-init.
> +            //
> +            // Link: https://github.com/Rust-for-Linux/pin-init/issues/59

You might want to add that this would avoid the extra allocation in
`DeviceRegistrationInner`.

> +            inner: {
> +                // SAFETY:
> +                //   - `this` is a valid pointer to `Self`,
> +                //   - `data` was properly initialized above.
> +                let data = unsafe { &*(*this.as_ptr()).data.get() };

As mentioned above, this creates a reference that is valid for this
*block*. So its lifetime will end after the `},` and before
`this_device` is initialized.

It *might* be ok to turn it back into a raw pointer in
`RawDeviceRegistration::new`, but I wouldn't bet on it.

> +
> +                let raw = RawDeviceRegistration::new(opts, parent, data);
> +
> +                // FIXME: Work around a bug in rustc, to prevent the following warning:
> +                //
> +                //   "warning: value captured by `dev` is never read."
> +                //
> +                // Link: https://github.com/rust-lang/rust/issues/141615

Note that the bug is that the compiler complains about the wrong span.
The original value of `dev` is `None` and that value is never used, so
the warning is justified. So this `let _ = dev;` still needs to stay
until `pin-init` supports accessing previously initialized fields (now
I'm pretty certain that I will implement that soon).

> +                let _ = dev;
> +
> +                if let Some(parent) = parent {
> +                    let devres = Devres::new(parent, raw, GFP_KERNEL)?;
> +
> +                    dev = Some(devres.access(parent)?.device().into());
> +                    DeviceRegistrationInner::Managed(devres)
> +                } else {
> +                    let boxed = KBox::pin_init(raw, GFP_KERNEL)?;
> +
> +                    dev = Some(boxed.device().into());
> +                    DeviceRegistrationInner::Raw(boxed)
> +                }
> +            },
> +            // Cache `this_device` within `Self` to avoid having to access `Devres` in the managed
> +            // case.
> +            this_device: {
> +                // SAFETY: `dev` is guaranteed to be set in the initializer of `inner` above.
> +                unsafe { dev.unwrap_unchecked() }
> +            },

No need for the extra block, just do:

    // Cache `this_device` within `Self` to avoid having to access `Devres` in the managed
    // case.
    // SAFETY: `dev` is guaranteed to be set in the initializer of `inner` above.
    this_device: unsafe { dev.unwrap_unchecked() },

I'm also pretty sure that the compiler would optimize `.take().unwrap()`
and also this is only executed once per `MiscDeviceRegistration`, so
even if it isn't it wouldn't really matter. So I'd prefer if we don't
use `unsafe` here even if it is painfully obvious (if I'm fast enough
with implementing, you can rebase on top before you merge and then this
will be gone anyways :)

> +            _t: PhantomData,
> +        })
> +    }
> +
> +    /// Access the `this_device` field.
> +    pub fn device(&self) -> &Device {
> +        &self.this_device
> +    }
> +
>      /// Access the additional data stored in this registration.
>      pub fn data(&self) -> &T::RegistrationData {
>          // SAFETY:
> @@ -120,9 +222,6 @@ pub fn data(&self) -> &T::RegistrationData {
>  #[pinned_drop]
>  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.
>          unsafe { core::ptr::drop_in_place(self.data.get()) };
>      }
> @@ -137,14 +236,13 @@ pub trait MiscDevice: Sized {
>      /// 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()`].
> +    /// This data can be accessed in [`MiscDevice::open()`].
>      type RegistrationData: Sync;
>  
>      /// Called when the misc device is opened.
>      ///
>      /// The returned pointer will be stored as the private data for the file.
> -    fn open(_file: &File, _misc: &MiscDeviceRegistration<Self>) -> Result<Self::Ptr>;
> +    fn open(_file: &File, _misc: &Device, _data: &Self::RegistrationData) -> Result<Self::Ptr>;

What is the reason that these parameters begin with `_`? In a trait
function without a body, the compiler shouldn't war about unused
parameters.

---
Cheers,
Benno

>  
>      /// Called when the misc device is released.
>      fn release(device: Self::Ptr, _file: &File) {

Re: [PATCH 1/7] rust: types: support fallible PinInit types in Opaque::pin_init

[Christian Schrefl]

On 30.05.25 9:29 PM, Benno Lossin wrote:
> On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
>> Currently, Opaque::pin_init only supports infallible PinInit
>> implementations, i.e. impl PinInit<T, Infallible>.
>>
>> This has been sufficient so far, since users such as Revocable do not
>> support fallibility.
>>
>> Since this is about to change, make Opaque::pin_init() generic over the
>> error type E.
>>
>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>> ---
>>  rust/kernel/types.rs | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>> index 22985b6f6982..75c99d6facf9 100644
>> --- a/rust/kernel/types.rs
>> +++ b/rust/kernel/types.rs
>> @@ -354,13 +354,13 @@ pub const fn zeroed() -> 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| {
>> +    pub fn pin_init<E>(slot: impl PinInit<T, E>) -> impl PinInit<Self, E> {
>> +        Self::try_ffi_init(|ptr: *mut T| -> Result<(), E> {
>>              // SAFETY:
>>              //   - `ptr` is a valid pointer to uninitialized memory,
>> -            //   - `slot` is not accessed on error; the call is infallible,
>> +            //   - `slot` is not accessed on error,
>>              //   - `slot` is pinned in memory.
>> -            let _ = unsafe { PinInit::<T>::__pinned_init(slot, ptr) };
>> +            unsafe { PinInit::<T, E>::__pinned_init(slot, ptr) }
> 
> Could you move this function into an `impl pin_init::Wrapper<T>` block?
> (it's the same function, but in a trait that was recently added)

This is then basically this patch [0] from my `UnsafePinned` series.
Just that I did not update the comment. :)

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

> 
> Thanks!
> 
> ---
> Cheers,
> Benno
> 
>>          })
>>      }
>>  
> 

Re: [PATCH 7/7] rust: sample: misc: implement device driver sample

[Benno Lossin]

On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
> In order to demonstrate and test a MiscDeviceRegistration with a parent
> device, introduce CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT.
>
> If CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT=y the misc device sample
> is initialized with a parent device (faux), otherwise it is initialized
> without a parent device, i.e. the exact same way as without this patch.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  samples/rust/Kconfig             |  8 +++++
>  samples/rust/rust_misc_device.rs | 50 +++++++++++++++++++++++++++++---
>  2 files changed, 54 insertions(+), 4 deletions(-)
>
> diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> index b1006ab4bc3c..9948ec0939ef 100644
> --- a/samples/rust/Kconfig
> +++ b/samples/rust/Kconfig
> @@ -30,6 +30,14 @@ config SAMPLE_RUST_MISC_DEVICE
>  
>  	  If unsure, say N.
>  
> +config SAMPLE_RUST_MISC_DEVICE_WITH_PARENT
> +	bool "Create a misc device with a parent device"
> +	depends on SAMPLE_RUST_MISC_DEVICE
> +	default n
> +	help
> +	  Say Y here if you want the misc device sample to create a misc
> +	  device with a parent device.
> +

Why not create a separate file? The `cfg`s might confuse newcomers
looking at the sample.

>  config SAMPLE_RUST_PRINT
>  	tristate "Printing macros"
>  	help
> diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> index 9bf1a0f64e6e..175638d6d341 100644
> --- a/samples/rust/rust_misc_device.rs
> +++ b/samples/rust/rust_misc_device.rs
> @@ -167,6 +167,9 @@
>      uaccess::{UserSlice, UserSliceReader, UserSliceWriter},
>  };
>  
> +#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
> +use kernel::faux;
> +
>  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);
> @@ -181,19 +184,33 @@
>      license: "GPL",
>  }
>  
> +#[cfg(not(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT))]
>  #[pin_data]
>  struct RustMiscDeviceModule {
>      #[pin]
>      _miscdev: MiscDeviceRegistration<RustMiscDevice>,
>  }
>  
> -impl kernel::InPlaceModule for RustMiscDeviceModule {
> -    fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
> +#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
> +struct RustMiscDeviceModule {
> +    _faux: faux::Registration,
> +    _miscdev: Pin<KBox<MiscDeviceRegistration<RustMiscDevice>>>,
> +}
> +
> +impl RustMiscDeviceModule {
> +    fn init() -> MiscDeviceOptions {
>          pr_info!("Initializing Rust Misc Device Sample\n");
>  
> -        let options = MiscDeviceOptions {
> +        MiscDeviceOptions {
>              name: c_str!("rust-misc-device"),
> -        };
> +        }
> +    }
> +}
> +
> +#[cfg(not(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT))]
> +impl kernel::InPlaceModule for RustMiscDeviceModule {
> +    fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
> +        let options = Self::init();
>  
>          try_pin_init!(Self {
>              _miscdev <- MiscDeviceRegistration::register(
> @@ -205,6 +222,31 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
>      }
>  }
>  
> +#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
> +impl kernel::Module for RustMiscDeviceModule {
> +    fn init(_module: &'static ThisModule) -> Result<Self> {
> +        let options = Self::init();
> +        let faux = faux::Registration::new(c_str!("rust-misc-device-sample"), None)?;
> +
> +        // For every other bus, this would be called from Driver::probe(), which would return a
> +        // `Result<Pin<KBox<T>>>`, but faux always binds to a "dummy" driver, hence probe() is

Not clear what `T` is supposed to be, do you mean `Self`?

> +        // not required.
> +        let misc = KBox::pin_init(
> +            MiscDeviceRegistration::register(
> +                options,
> +                Arc::pin_init(new_mutex!(Inner { value: 0_i32 }), GFP_KERNEL),
> +                Some(faux.as_ref()),
> +            ),
> +            GFP_KERNEL,
> +        )?;

You could also initialize this module variation in-place. (this would
also require the pin-init change to reference initialized fields)

---
Cheers,
Benno

> +
> +        Ok(Self {
> +            _faux: faux,
> +            _miscdev: misc,
> +        })
> +    }
> +}
> +
>  struct Inner {
>      value: i32,
>  }

Re: [PATCH 1/7] rust: types: support fallible PinInit types in Opaque::pin_init

[Benno Lossin]

On Fri May 30, 2025 at 10:11 PM CEST, Christian Schrefl wrote:
> On 30.05.25 9:29 PM, Benno Lossin wrote:
>> On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
>>> -    pub fn pin_init(slot: impl PinInit<T>) -> impl PinInit<Self> {
>>> -        Self::ffi_init(|ptr: *mut T| {
>>> +    pub fn pin_init<E>(slot: impl PinInit<T, E>) -> impl PinInit<Self, E> {
>>> +        Self::try_ffi_init(|ptr: *mut T| -> Result<(), E> {
>>>              // SAFETY:
>>>              //   - `ptr` is a valid pointer to uninitialized memory,
>>> -            //   - `slot` is not accessed on error; the call is infallible,
>>> +            //   - `slot` is not accessed on error,
>>>              //   - `slot` is pinned in memory.
>>> -            let _ = unsafe { PinInit::<T>::__pinned_init(slot, ptr) };
>>> +            unsafe { PinInit::<T, E>::__pinned_init(slot, ptr) }
>> 
>> Could you move this function into an `impl pin_init::Wrapper<T>` block?
>> (it's the same function, but in a trait that was recently added)
>
> This is then basically this patch [0] from my `UnsafePinned` series.
> Just that I did not update the comment. :)
>
> [0]: https://lore.kernel.org/rust-for-linux/20250511-rust_unsafe_pinned-v4-2-a86c32e47e3d@gmail.com/ 

Oh yeah, I completely forgot we had this... I even reviewed it haha!

---
Cheers,
Benno

Re: [PATCH 1/7] rust: types: support fallible PinInit types in Opaque::pin_init

[Danilo Krummrich]

On Fri, May 30, 2025 at 10:11:00PM +0200, Christian Schrefl wrote:
> 
> 
> On 30.05.25 9:29 PM, Benno Lossin wrote:
> > On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
> >> Currently, Opaque::pin_init only supports infallible PinInit
> >> implementations, i.e. impl PinInit<T, Infallible>.
> >>
> >> This has been sufficient so far, since users such as Revocable do not
> >> support fallibility.
> >>
> >> Since this is about to change, make Opaque::pin_init() generic over the
> >> error type E.
> >>
> >> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> >> ---
> >>  rust/kernel/types.rs | 8 ++++----
> >>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> >> index 22985b6f6982..75c99d6facf9 100644
> >> --- a/rust/kernel/types.rs
> >> +++ b/rust/kernel/types.rs
> >> @@ -354,13 +354,13 @@ pub const fn zeroed() -> 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| {
> >> +    pub fn pin_init<E>(slot: impl PinInit<T, E>) -> impl PinInit<Self, E> {
> >> +        Self::try_ffi_init(|ptr: *mut T| -> Result<(), E> {
> >>              // SAFETY:
> >>              //   - `ptr` is a valid pointer to uninitialized memory,
> >> -            //   - `slot` is not accessed on error; the call is infallible,
> >> +            //   - `slot` is not accessed on error,
> >>              //   - `slot` is pinned in memory.
> >> -            let _ = unsafe { PinInit::<T>::__pinned_init(slot, ptr) };
> >> +            unsafe { PinInit::<T, E>::__pinned_init(slot, ptr) }
> > 
> > Could you move this function into an `impl pin_init::Wrapper<T>` block?
> > (it's the same function, but in a trait that was recently added)
> 
> This is then basically this patch [0] from my `UnsafePinned` series.
> Just that I did not update the comment. :)

As mentioned in [0], I wasn't aware of this patch -- let's go with yours then.

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

Re: [PATCH 5/7] rust: miscdevice: properly support device drivers

[Danilo Krummrich]

On Fri, May 30, 2025 at 10:06:55PM +0200, Benno Lossin wrote:
> On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
> > @@ -40,44 +41,43 @@ pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
> >      }
> >  }
> >  
> > -/// A registration of a miscdevice.
> > -///
> >  /// # Invariants
> >  ///
> > -/// `inner` is a registered misc device.
> > +/// - `inner` is a registered misc device,
> > +/// - `data` is valid for the entire lifetime of `Self`.
> >  #[repr(C)]
> >  #[pin_data(PinnedDrop)]
> > -pub struct MiscDeviceRegistration<T: MiscDevice> {
> > +struct RawDeviceRegistration<T: MiscDevice> {
> >      #[pin]
> >      inner: Opaque<bindings::miscdevice>,
> > -    #[pin]
> > -    data: Opaque<T::RegistrationData>,
> > +    data: NonNull<T::RegistrationData>,
> >      _t: PhantomData<T>,
> 
> You shouldn't need the `PhantomData` here.
> 
> Also, do we need to ask for `T: MiscDevice` here? Could we instead have
> just `T` and then below you write
> `RawDeviceRegistration<T::RegistrationData>` instead? (`new` of course
> needs to have a new generic: `U: MiscDevice<RegistrationData = T>`)

Sure, is there any advantage? Your proposal seems more complicated at a first
glance.

> >  }
> >  
> > -// 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(
> > +impl<T: MiscDevice> RawDeviceRegistration<T> {
> > +    fn new<'a>(
> >          opts: MiscDeviceOptions,
> > -        data: impl PinInit<T::RegistrationData, Error>,
> > -    ) -> impl PinInit<Self, Error> {
> > +        parent: Option<&'a Device<Bound>>,
> > +        data: &'a T::RegistrationData,
> > +    ) -> impl PinInit<Self, Error> + 'a
> > +    where
> > +        T: 'a,
> > +    {
> >          try_pin_init!(Self {
> > -            data <- Opaque::pin_init(data),
> > +            // INVARIANT: `Self` is always embedded in a `MiscDeviceRegistration<T>`, hence `data`
> > +            // is guaranteed to be valid for the entire lifetime of `Self`.
> > +            data: NonNull::from(data),
> 
> Both the argument in the INVARIANT comment and way this works are a bit
> flawed.

Why is the argument flawed? Let's say we go with your proposal below, what would
the safety requirement for RawDeviceRegistration::new and the invariant of
RawDeviceRegistration look like? Wouldn't it be the exact same argument?

> Instead, I'd recommend directly taking the `NonNull` as a
> parameter. Yes the function will need to be `unsafe`, but the lifetime
> that you're creating below only lives for `'a`, but the object might
> live much longer. You might still be fine, but I'd just recommend
> staying in raw pointer land (or in this case `NonNull`).
> 
> >              inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
> > +                let mut value = opts.into_raw::<T>();
> > +
> > +                if let Some(parent) = parent {
> > +                    // The device core code will take care to take a reference of `parent` in
> 
> Just a question: with "take a reference of" you mean that it will
> increment the refcount?

Exactly -- will change it to "increment the refcount" for clarity.

> 
> > +                    // `device_add()` called by `misc_register()`.
> > +                    value.parent = parent.as_raw();
> > +                }
> > +
> >                  // SAFETY: The initializer can write to the provided `slot`.
> > -                unsafe { slot.write(opts.into_raw::<T>()) };
> > +                unsafe { slot.write(value) };
> >  
> >                  // SAFETY:
> >                  // * We just wrote the misc device options to the slot. The miscdevice will
> > @@ -94,12 +94,12 @@ pub fn register(
> >      }
> >  
> >      /// Returns a raw pointer to the misc device.
> > -    pub fn as_raw(&self) -> *mut bindings::miscdevice {
> > +    fn as_raw(&self) -> *mut bindings::miscdevice {
> >          self.inner.get()
> >      }
> >  
> >      /// Access the `this_device` field.
> > -    pub fn device(&self) -> &Device {
> > +    fn device(&self) -> &Device {
> >          // SAFETY: This can only be called after a successful register(), which always
> >          // initialises `this_device` with a valid device. Furthermore, the signature of this
> >          // function tells the borrow-checker that the `&Device` reference must not outlive the
> > @@ -108,6 +108,108 @@ pub fn device(&self) -> &Device {
> >          unsafe { Device::as_ref((*self.as_raw()).this_device) }
> >      }
> >  
> > +    fn data(&self) -> &T::RegistrationData {
> > +        // SAFETY: The type invariant guarantees that `data` is valid for the entire lifetime of
> > +        // `Self`.
> > +        unsafe { self.data.as_ref() }
> > +    }
> > +}
> > +
> > +#[pinned_drop]
> > +impl<T: MiscDevice> PinnedDrop for RawDeviceRegistration<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()) };
> > +    }
> > +}
> > +
> > +#[expect(dead_code)]
> > +enum DeviceRegistrationInner<T: MiscDevice> {
> > +    Raw(Pin<KBox<RawDeviceRegistration<T>>>),
> > +    Managed(Devres<RawDeviceRegistration<T>>),
> 
> These two names could be shortened (`DeviceRegistrationInner` and
> `RawDeviceRegistration`) as they are only implementation details of this
> file. How about `InnerRegistration` and `RawRegistration`? Or maybe
> something even shorter.

There's a reason why I keep them something with "DeviceRegistration" everywhere,
which is to make it clear that it's both a device instance *and* a registration,
which is actually rather uncommon and caused by the fact that device creation
and registration needs to be done under the misc_mtx in misc_register().

This is also the reason for those data structures to be a bit complicated; it
would be much simpler if device creation and registration would be independent
things.

> > +}
> > +
> > +/// A registration of a miscdevice.
> > +#[pin_data(PinnedDrop)]
> > +pub struct MiscDeviceRegistration<T: MiscDevice> {
> > +    inner: DeviceRegistrationInner<T>,
> > +    #[pin]
> > +    data: Opaque<T::RegistrationData>,
> 
> Why is it necessary to store `data` inside of `Opaque`?

It was UnsafePinned before, but Alice proposed to go with Opaque for the
meantime. Anyways, this is not introduced by this patch, it comes from
Christians patch adding T::RegistrationData.

> 
> > +    this_device: ARef<Device>,
> > +    _t: PhantomData<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<'a>(
> > +        opts: MiscDeviceOptions,
> > +        data: impl PinInit<T::RegistrationData, Error> + 'a,
> > +        parent: Option<&'a Device<Bound>>,
> > +    ) -> impl PinInit<Self, Error> + 'a
> > +    where
> > +        T: 'a,
> > +    {
> > +        let mut dev: Option<ARef<Device>> = None;
> > +
> > +        try_pin_init!(&this in Self {
> > +            data <- Opaque::pin_init(data),
> > +            // TODO: make `inner` in-place when enums get supported by pin-init.
> > +            //
> > +            // Link: https://github.com/Rust-for-Linux/pin-init/issues/59
> 
> You might want to add that this would avoid the extra allocation in
> `DeviceRegistrationInner`.

Sure, will do.

> > +            inner: {
> > +                // SAFETY:
> > +                //   - `this` is a valid pointer to `Self`,
> > +                //   - `data` was properly initialized above.
> > +                let data = unsafe { &*(*this.as_ptr()).data.get() };
> 
> As mentioned above, this creates a reference that is valid for this
> *block*. So its lifetime will end after the `},` and before
> `this_device` is initialized.
> 
> It *might* be ok to turn it back into a raw pointer in
> `RawDeviceRegistration::new`, but I wouldn't bet on it.

Why? The reference is still valid in RawDeviceRegistration::new, no?

> > +
> > +                let raw = RawDeviceRegistration::new(opts, parent, data);
> > +
> > +                // FIXME: Work around a bug in rustc, to prevent the following warning:
> > +                //
> > +                //   "warning: value captured by `dev` is never read."
> > +                //
> > +                // Link: https://github.com/rust-lang/rust/issues/141615
> 
> Note that the bug is that the compiler complains about the wrong span.
> The original value of `dev` is `None` and that value is never used, so
> the warning is justified. So this `let _ = dev;` still needs to stay
> until `pin-init` supports accessing previously initialized fields (now
> I'm pretty certain that I will implement that soon).

Do you want to propose an alternative comment about this?

> > +                let _ = dev;
> > +
> > +                if let Some(parent) = parent {
> > +                    let devres = Devres::new(parent, raw, GFP_KERNEL)?;
> > +
> > +                    dev = Some(devres.access(parent)?.device().into());
> > +                    DeviceRegistrationInner::Managed(devres)
> > +                } else {
> > +                    let boxed = KBox::pin_init(raw, GFP_KERNEL)?;
> > +
> > +                    dev = Some(boxed.device().into());
> > +                    DeviceRegistrationInner::Raw(boxed)
> > +                }
> > +            },
> > +            // Cache `this_device` within `Self` to avoid having to access `Devres` in the managed
> > +            // case.
> > +            this_device: {
> > +                // SAFETY: `dev` is guaranteed to be set in the initializer of `inner` above.
> > +                unsafe { dev.unwrap_unchecked() }
> > +            },
> 
> No need for the extra block, just do:
> 
>     // Cache `this_device` within `Self` to avoid having to access `Devres` in the managed
>     // case.
>     // SAFETY: `dev` is guaranteed to be set in the initializer of `inner` above.
>     this_device: unsafe { dev.unwrap_unchecked() },

Yes, I know, but I found the above a bit cleaner -- I don't mind changing it
though.

> I'm also pretty sure that the compiler would optimize `.take().unwrap()`
> and also this is only executed once per `MiscDeviceRegistration`, so
> even if it isn't it wouldn't really matter. So I'd prefer if we don't
> use `unsafe` here even if it is painfully obvious (if I'm fast enough
> with implementing, you can rebase on top before you merge and then this
> will be gone anyways :)

Sounds good! :)

But I think that unsafe is better than unwrap() in such cases; unsafe requires
us to explain why it's OK to do it, which makes it less likely to create bugs.

(Just recently I wrote some code, hit the need for unsafe and, while writing up
the safety comment, I had to explain to myself, why the way I was about to
implement this was pretty broken.)

unwrap() on the other hand, doesn't require any explanation, but panics the
kernel in the worst case.

> > +            _t: PhantomData,
> > +        })
> > +    }
> > +
> > +    /// Access the `this_device` field.
> > +    pub fn device(&self) -> &Device {
> > +        &self.this_device
> > +    }
> > +
> >      /// Access the additional data stored in this registration.
> >      pub fn data(&self) -> &T::RegistrationData {
> >          // SAFETY:
> > @@ -120,9 +222,6 @@ pub fn data(&self) -> &T::RegistrationData {
> >  #[pinned_drop]
> >  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.
> >          unsafe { core::ptr::drop_in_place(self.data.get()) };
> >      }
> > @@ -137,14 +236,13 @@ pub trait MiscDevice: Sized {
> >      /// 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()`].
> > +    /// This data can be accessed in [`MiscDevice::open()`].
> >      type RegistrationData: Sync;
> >  
> >      /// Called when the misc device is opened.
> >      ///
> >      /// The returned pointer will be stored as the private data for the file.
> > -    fn open(_file: &File, _misc: &MiscDeviceRegistration<Self>) -> Result<Self::Ptr>;
> > +    fn open(_file: &File, _misc: &Device, _data: &Self::RegistrationData) -> Result<Self::Ptr>;
> 
> What is the reason that these parameters begin with `_`? In a trait
> function without a body, the compiler shouldn't war about unused
> parameters.

No idea, I just tried to be complient with the existing style of the file. :)

Re: [PATCH 7/7] rust: sample: misc: implement device driver sample

[Danilo Krummrich]

On Fri, May 30, 2025 at 10:15:37PM +0200, Benno Lossin wrote:
> On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
> > In order to demonstrate and test a MiscDeviceRegistration with a parent
> > device, introduce CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT.
> >
> > If CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT=y the misc device sample
> > is initialized with a parent device (faux), otherwise it is initialized
> > without a parent device, i.e. the exact same way as without this patch.
> >
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > ---
> >  samples/rust/Kconfig             |  8 +++++
> >  samples/rust/rust_misc_device.rs | 50 +++++++++++++++++++++++++++++---
> >  2 files changed, 54 insertions(+), 4 deletions(-)
> >
> > diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> > index b1006ab4bc3c..9948ec0939ef 100644
> > --- a/samples/rust/Kconfig
> > +++ b/samples/rust/Kconfig
> > @@ -30,6 +30,14 @@ config SAMPLE_RUST_MISC_DEVICE
> >  
> >  	  If unsure, say N.
> >  
> > +config SAMPLE_RUST_MISC_DEVICE_WITH_PARENT
> > +	bool "Create a misc device with a parent device"
> > +	depends on SAMPLE_RUST_MISC_DEVICE
> > +	default n
> > +	help
> > +	  Say Y here if you want the misc device sample to create a misc
> > +	  device with a parent device.
> > +
> 
> Why not create a separate file? The `cfg`s might confuse newcomers
> looking at the sample.

It would be a lot of duplicated code, unless we really *only* exercise the
device creation and registration part, which would be a bit unfortunate, given
that this sample is also a pretty good test.

> >  config SAMPLE_RUST_PRINT
> >  	tristate "Printing macros"
> >  	help
> > diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> > index 9bf1a0f64e6e..175638d6d341 100644
> > --- a/samples/rust/rust_misc_device.rs
> > +++ b/samples/rust/rust_misc_device.rs
> > @@ -167,6 +167,9 @@
> >      uaccess::{UserSlice, UserSliceReader, UserSliceWriter},
> >  };
> >  
> > +#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
> > +use kernel::faux;
> > +
> >  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);
> > @@ -181,19 +184,33 @@
> >      license: "GPL",
> >  }
> >  
> > +#[cfg(not(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT))]
> >  #[pin_data]
> >  struct RustMiscDeviceModule {
> >      #[pin]
> >      _miscdev: MiscDeviceRegistration<RustMiscDevice>,
> >  }
> >  
> > -impl kernel::InPlaceModule for RustMiscDeviceModule {
> > -    fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
> > +#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
> > +struct RustMiscDeviceModule {
> > +    _faux: faux::Registration,
> > +    _miscdev: Pin<KBox<MiscDeviceRegistration<RustMiscDevice>>>,
> > +}
> > +
> > +impl RustMiscDeviceModule {
> > +    fn init() -> MiscDeviceOptions {
> >          pr_info!("Initializing Rust Misc Device Sample\n");
> >  
> > -        let options = MiscDeviceOptions {
> > +        MiscDeviceOptions {
> >              name: c_str!("rust-misc-device"),
> > -        };
> > +        }
> > +    }
> > +}
> > +
> > +#[cfg(not(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT))]
> > +impl kernel::InPlaceModule for RustMiscDeviceModule {
> > +    fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
> > +        let options = Self::init();
> >  
> >          try_pin_init!(Self {
> >              _miscdev <- MiscDeviceRegistration::register(
> > @@ -205,6 +222,31 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
> >      }
> >  }
> >  
> > +#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
> > +impl kernel::Module for RustMiscDeviceModule {
> > +    fn init(_module: &'static ThisModule) -> Result<Self> {
> > +        let options = Self::init();
> > +        let faux = faux::Registration::new(c_str!("rust-misc-device-sample"), None)?;
> > +
> > +        // For every other bus, this would be called from Driver::probe(), which would return a
> > +        // `Result<Pin<KBox<T>>>`, but faux always binds to a "dummy" driver, hence probe() is
> 
> Not clear what `T` is supposed to be, do you mean `Self`?

From the perspective of the type implementing the corresponding Driver trait it
would indeed be `Self`. But I found it ambiguous to write `Self`, since I do *not*
mean `RustMiscDeviceModule` with `Self`.

> > +        // not required.
> > +        let misc = KBox::pin_init(
> > +            MiscDeviceRegistration::register(
> > +                options,
> > +                Arc::pin_init(new_mutex!(Inner { value: 0_i32 }), GFP_KERNEL),
> > +                Some(faux.as_ref()),
> > +            ),
> > +            GFP_KERNEL,
> > +        )?;
> 
> You could also initialize this module variation in-place. (this would
> also require the pin-init change to reference initialized fields)

Yes, I also thought about that. But this way is a bit closer to what things
would look like within a probe() callback.

Re: [PATCH 5/7] rust: miscdevice: properly support device drivers

[Benno Lossin]

On Sat May 31, 2025 at 12:17 AM CEST, Danilo Krummrich wrote:
> On Fri, May 30, 2025 at 10:06:55PM +0200, Benno Lossin wrote:
>> On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
>> > @@ -40,44 +41,43 @@ pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
>> >      }
>> >  }
>> >  
>> > -/// A registration of a miscdevice.
>> > -///
>> >  /// # Invariants
>> >  ///
>> > -/// `inner` is a registered misc device.
>> > +/// - `inner` is a registered misc device,
>> > +/// - `data` is valid for the entire lifetime of `Self`.
>> >  #[repr(C)]
>> >  #[pin_data(PinnedDrop)]
>> > -pub struct MiscDeviceRegistration<T: MiscDevice> {
>> > +struct RawDeviceRegistration<T: MiscDevice> {
>> >      #[pin]
>> >      inner: Opaque<bindings::miscdevice>,
>> > -    #[pin]
>> > -    data: Opaque<T::RegistrationData>,
>> > +    data: NonNull<T::RegistrationData>,
>> >      _t: PhantomData<T>,
>> 
>> You shouldn't need the `PhantomData` here.
>> 
>> Also, do we need to ask for `T: MiscDevice` here? Could we instead have
>> just `T` and then below you write
>> `RawDeviceRegistration<T::RegistrationData>` instead? (`new` of course
>> needs to have a new generic: `U: MiscDevice<RegistrationData = T>`)
>
> Sure, is there any advantage? Your proposal seems more complicated at a first
> glance.

It would make `RawDeviceRegistration` simpler, but maybe it's not worth
it.

>> >  }
>> >  
>> > -// 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(
>> > +impl<T: MiscDevice> RawDeviceRegistration<T> {
>> > +    fn new<'a>(
>> >          opts: MiscDeviceOptions,
>> > -        data: impl PinInit<T::RegistrationData, Error>,
>> > -    ) -> impl PinInit<Self, Error> {
>> > +        parent: Option<&'a Device<Bound>>,
>> > +        data: &'a T::RegistrationData,
>> > +    ) -> impl PinInit<Self, Error> + 'a
>> > +    where
>> > +        T: 'a,
>> > +    {
>> >          try_pin_init!(Self {
>> > -            data <- Opaque::pin_init(data),
>> > +            // INVARIANT: `Self` is always embedded in a `MiscDeviceRegistration<T>`, hence `data`
>> > +            // is guaranteed to be valid for the entire lifetime of `Self`.
>> > +            data: NonNull::from(data),
>> 
>> Both the argument in the INVARIANT comment and way this works are a bit
>> flawed.
>
> Why is the argument flawed? Let's say we go with your proposal below, what would
> the safety requirement for RawDeviceRegistration::new and the invariant of
> RawDeviceRegistration look like? Wouldn't it be the exact same argument?

So what I write below it not really true. But the argument relies on the
fact that `data` is pointing to a value that will stay alive for the
duration of the existence of `self`. That should be a safety requirement
of `new` (even if we take a reference as an argument).

>> Instead, I'd recommend directly taking the `NonNull` as a
>> parameter. Yes the function will need to be `unsafe`, but the lifetime
>> that you're creating below only lives for `'a`, but the object might
>> live much longer. You might still be fine, but I'd just recommend
>> staying in raw pointer land (or in this case `NonNull`).

>> > @@ -108,6 +108,108 @@ pub fn device(&self) -> &Device {
>> >          unsafe { Device::as_ref((*self.as_raw()).this_device) }
>> >      }
>> >  
>> > +    fn data(&self) -> &T::RegistrationData {
>> > +        // SAFETY: The type invariant guarantees that `data` is valid for the entire lifetime of
>> > +        // `Self`.
>> > +        unsafe { self.data.as_ref() }
>> > +    }
>> > +}
>> > +
>> > +#[pinned_drop]
>> > +impl<T: MiscDevice> PinnedDrop for RawDeviceRegistration<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()) };
>> > +    }
>> > +}
>> > +
>> > +#[expect(dead_code)]
>> > +enum DeviceRegistrationInner<T: MiscDevice> {
>> > +    Raw(Pin<KBox<RawDeviceRegistration<T>>>),
>> > +    Managed(Devres<RawDeviceRegistration<T>>),
>> 
>> These two names could be shortened (`DeviceRegistrationInner` and
>> `RawDeviceRegistration`) as they are only implementation details of this
>> file. How about `InnerRegistration` and `RawRegistration`? Or maybe
>> something even shorter.
>
> There's a reason why I keep them something with "DeviceRegistration" everywhere,
> which is to make it clear that it's both a device instance *and* a registration,
> which is actually rather uncommon and caused by the fact that device creation
> and registration needs to be done under the misc_mtx in misc_register().
>
> This is also the reason for those data structures to be a bit complicated; it
> would be much simpler if device creation and registration would be independent
> things.

Then keep the longer names.

>> > +}
>> > +
>> > +/// A registration of a miscdevice.
>> > +#[pin_data(PinnedDrop)]
>> > +pub struct MiscDeviceRegistration<T: MiscDevice> {
>> > +    inner: DeviceRegistrationInner<T>,
>> > +    #[pin]
>> > +    data: Opaque<T::RegistrationData>,
>> 
>> Why is it necessary to store `data` inside of `Opaque`?
>
> It was UnsafePinned before, but Alice proposed to go with Opaque for the
> meantime. Anyways, this is not introduced by this patch, it comes from
> Christians patch adding T::RegistrationData.

Ah then I'll re-read that discussion.

>> > +    this_device: ARef<Device>,
>> > +    _t: PhantomData<T>,
>> > +}

>> > +            inner: {
>> > +                // SAFETY:
>> > +                //   - `this` is a valid pointer to `Self`,
>> > +                //   - `data` was properly initialized above.
>> > +                let data = unsafe { &*(*this.as_ptr()).data.get() };
>> 
>> As mentioned above, this creates a reference that is valid for this
>> *block*. So its lifetime will end after the `},` and before
>> `this_device` is initialized.
>> 
>> It *might* be ok to turn it back into a raw pointer in
>> `RawDeviceRegistration::new`, but I wouldn't bet on it.
>
> Why? The reference is still valid in RawDeviceRegistration::new, no?

Yes the reference is still valid in the `new` function call. I was under
the impression that the pointer created in `new` would get invalidated,
because the struct would get reborrowed in the meantime, but that's not
the case, since this pointer is obtained by `get`. So you should be
good.

>> > +
>> > +                let raw = RawDeviceRegistration::new(opts, parent, data);
>> > +
>> > +                // FIXME: Work around a bug in rustc, to prevent the following warning:
>> > +                //
>> > +                //   "warning: value captured by `dev` is never read."
>> > +                //
>> > +                // Link: https://github.com/rust-lang/rust/issues/141615
>> 
>> Note that the bug is that the compiler complains about the wrong span.
>> The original value of `dev` is `None` and that value is never used, so
>> the warning is justified. So this `let _ = dev;` still needs to stay
>> until `pin-init` supports accessing previously initialized fields (now
>> I'm pretty certain that I will implement that soon).
>
> Do you want to propose an alternative comment about this?

I think we don't need a comment here.

>> > +                let _ = dev;
>> > +
>> > +                if let Some(parent) = parent {
>> > +                    let devres = Devres::new(parent, raw, GFP_KERNEL)?;
>> > +
>> > +                    dev = Some(devres.access(parent)?.device().into());
>> > +                    DeviceRegistrationInner::Managed(devres)
>> > +                } else {
>> > +                    let boxed = KBox::pin_init(raw, GFP_KERNEL)?;
>> > +
>> > +                    dev = Some(boxed.device().into());
>> > +                    DeviceRegistrationInner::Raw(boxed)
>> > +                }
>> > +            },
>> > +            // Cache `this_device` within `Self` to avoid having to access `Devres` in the managed
>> > +            // case.
>> > +            this_device: {
>> > +                // SAFETY: `dev` is guaranteed to be set in the initializer of `inner` above.
>> > +                unsafe { dev.unwrap_unchecked() }
>> > +            },
>> 
>> No need for the extra block, just do:
>> 
>>     // Cache `this_device` within `Self` to avoid having to access `Devres` in the managed
>>     // case.
>>     // SAFETY: `dev` is guaranteed to be set in the initializer of `inner` above.
>>     this_device: unsafe { dev.unwrap_unchecked() },
>
> Yes, I know, but I found the above a bit cleaner -- I don't mind changing it
> though.
>
>> I'm also pretty sure that the compiler would optimize `.take().unwrap()`
>> and also this is only executed once per `MiscDeviceRegistration`, so
>> even if it isn't it wouldn't really matter. So I'd prefer if we don't
>> use `unsafe` here even if it is painfully obvious (if I'm fast enough
>> with implementing, you can rebase on top before you merge and then this
>> will be gone anyways :)
>
> Sounds good! :)
>
> But I think that unsafe is better than unwrap() in such cases; unsafe requires
> us to explain why it's OK to do it, which makes it less likely to create bugs.

That's not exactly how I would think about it, but your argument makes
sense.

> (Just recently I wrote some code, hit the need for unsafe and, while writing up
> the safety comment, I had to explain to myself, why the way I was about to
> implement this was pretty broken.)

That's great :)

> unwrap() on the other hand, doesn't require any explanation, but panics the
> kernel in the worst case.

Yeah that is true. Ultimately for this case it won't matter, since I'll
just implement the access thing in pin-init.

It'll still take a while, because it will touch several parts that other
patches also touch. Thus I prefer to first pick the patches already on
the list, but for that I'll wait until the merge window closes.

---
Cheers,
Benno

Re: [PATCH 7/7] rust: sample: misc: implement device driver sample

[Benno Lossin]

On Sat May 31, 2025 at 12:24 AM CEST, Danilo Krummrich wrote:
> On Fri, May 30, 2025 at 10:15:37PM +0200, Benno Lossin wrote:
>> On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
>> > In order to demonstrate and test a MiscDeviceRegistration with a parent
>> > device, introduce CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT.
>> >
>> > If CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT=y the misc device sample
>> > is initialized with a parent device (faux), otherwise it is initialized
>> > without a parent device, i.e. the exact same way as without this patch.
>> >
>> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>> > ---
>> >  samples/rust/Kconfig             |  8 +++++
>> >  samples/rust/rust_misc_device.rs | 50 +++++++++++++++++++++++++++++---
>> >  2 files changed, 54 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
>> > index b1006ab4bc3c..9948ec0939ef 100644
>> > --- a/samples/rust/Kconfig
>> > +++ b/samples/rust/Kconfig
>> > @@ -30,6 +30,14 @@ config SAMPLE_RUST_MISC_DEVICE
>> >  
>> >  	  If unsure, say N.
>> >  
>> > +config SAMPLE_RUST_MISC_DEVICE_WITH_PARENT
>> > +	bool "Create a misc device with a parent device"
>> > +	depends on SAMPLE_RUST_MISC_DEVICE
>> > +	default n
>> > +	help
>> > +	  Say Y here if you want the misc device sample to create a misc
>> > +	  device with a parent device.
>> > +
>> 
>> Why not create a separate file? The `cfg`s might confuse newcomers
>> looking at the sample.
>
> It would be a lot of duplicated code, unless we really *only* exercise the
> device creation and registration part, which would be a bit unfortunate, given
> that this sample is also a pretty good test.

We could separate the common parts into a single file and then
`include!` that file from the two samples. (Or if the build system
supports multi-file samples then just use that, but my gut feeling is
that it doesn't)

I really would like to avoid `cfg` in samples.

>> >  config SAMPLE_RUST_PRINT
>> >  	tristate "Printing macros"
>> >  	help
>> > diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
>> > index 9bf1a0f64e6e..175638d6d341 100644
>> > --- a/samples/rust/rust_misc_device.rs
>> > +++ b/samples/rust/rust_misc_device.rs
>> > @@ -167,6 +167,9 @@
>> >      uaccess::{UserSlice, UserSliceReader, UserSliceWriter},
>> >  };
>> >  
>> > +#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
>> > +use kernel::faux;
>> > +
>> >  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);
>> > @@ -181,19 +184,33 @@
>> >      license: "GPL",
>> >  }
>> >  
>> > +#[cfg(not(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT))]
>> >  #[pin_data]
>> >  struct RustMiscDeviceModule {
>> >      #[pin]
>> >      _miscdev: MiscDeviceRegistration<RustMiscDevice>,
>> >  }
>> >  
>> > -impl kernel::InPlaceModule for RustMiscDeviceModule {
>> > -    fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
>> > +#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
>> > +struct RustMiscDeviceModule {
>> > +    _faux: faux::Registration,
>> > +    _miscdev: Pin<KBox<MiscDeviceRegistration<RustMiscDevice>>>,
>> > +}
>> > +
>> > +impl RustMiscDeviceModule {
>> > +    fn init() -> MiscDeviceOptions {
>> >          pr_info!("Initializing Rust Misc Device Sample\n");
>> >  
>> > -        let options = MiscDeviceOptions {
>> > +        MiscDeviceOptions {
>> >              name: c_str!("rust-misc-device"),
>> > -        };
>> > +        }
>> > +    }
>> > +}
>> > +
>> > +#[cfg(not(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT))]
>> > +impl kernel::InPlaceModule for RustMiscDeviceModule {
>> > +    fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
>> > +        let options = Self::init();
>> >  
>> >          try_pin_init!(Self {
>> >              _miscdev <- MiscDeviceRegistration::register(
>> > @@ -205,6 +222,31 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
>> >      }
>> >  }
>> >  
>> > +#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
>> > +impl kernel::Module for RustMiscDeviceModule {
>> > +    fn init(_module: &'static ThisModule) -> Result<Self> {
>> > +        let options = Self::init();
>> > +        let faux = faux::Registration::new(c_str!("rust-misc-device-sample"), None)?;
>> > +
>> > +        // For every other bus, this would be called from Driver::probe(), which would return a

Missing '`' around Driver::probe().

>> > +        // `Result<Pin<KBox<T>>>`, but faux always binds to a "dummy" driver, hence probe() is
>> 
>> Not clear what `T` is supposed to be, do you mean `Self`?
>
> From the perspective of the type implementing the corresponding Driver trait it
> would indeed be `Self`. But I found it ambiguous to write `Self`, since I do *not*
> mean `RustMiscDeviceModule` with `Self`.

Yeah that makes sense, I already entered into the `impl Driver` context
:) How about we use `<T as Driver>::probe()` above and then `T` makes
sense?

Another thing: faux devices don't have a `probe` in rust, so saying "not
required" doesn't make much sense, right?

>> > +        // not required.
>> > +        let misc = KBox::pin_init(
>> > +            MiscDeviceRegistration::register(
>> > +                options,
>> > +                Arc::pin_init(new_mutex!(Inner { value: 0_i32 }), GFP_KERNEL),
>> > +                Some(faux.as_ref()),
>> > +            ),
>> > +            GFP_KERNEL,
>> > +        )?;
>> 
>> You could also initialize this module variation in-place. (this would
>> also require the pin-init change to reference initialized fields)
>
> Yes, I also thought about that. But this way is a bit closer to what things
> would look like within a probe() callback.

Yeah then let's do that :)

---
Cheers,
Benno

Re: [PATCH 6/7] rust: miscdevice: expose the parent device as &Device<Bound>

[Benno Lossin]

On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
> @@ -227,11 +229,21 @@ fn drop(self: Pin<&mut Self>) {
>      }
>  }
>  
> +/// The arguments passed to the file operation callbacks of a [`MiscDeviceRegistration`].
> +pub struct MiscArgs<'a, T: MiscDevice> {
> +    /// The [`Device`] representation of the `struct miscdevice`.
> +    pub device: &'a Device,
> +    /// The parent [`Device`] of [`Self::device`].
> +    pub parent: Option<&'a Device<Bound>>,
> +    /// The `RegistrationData` passed to [`MiscDeviceRegistration::register`].
> +    pub data: &'a T::RegistrationData,

Here I would also just use `T`, remove the `MiscDevice` bound and then
use `MiscArgs<'_, Self::RegistrationData>` below.

> +}
> +
>  /// Trait implemented by the private data of an open misc device.
>  #[vtable]
>  pub trait MiscDevice: Sized {
>      /// What kind of pointer should `Self` be wrapped in.
> -    type Ptr: ForeignOwnable + Send + Sync;
> +    type Ptr: Send + Sync;

There is no info about this change in the commit message. Why are we
changing this? This seems a bit orthogonal to the other change, maybe do
it in a separate patch?

Also `Ptr` doesn't make much sense for the name, since now that the
`ForeignOwnable` bound is gone, I could set this to `Self` and then have
access to `&Self` in the callbacks.

Would that also make sense to use as a general change? So don't store
`Self::Ptr`, but `Self` directly?

---
Cheers,
Benno

Re: [PATCH 7/7] rust: sample: misc: implement device driver sample

[Danilo Krummrich]

On Sat, May 31, 2025 at 10:11:08AM +0200, Benno Lossin wrote:
> On Sat May 31, 2025 at 12:24 AM CEST, Danilo Krummrich wrote:
> > On Fri, May 30, 2025 at 10:15:37PM +0200, Benno Lossin wrote:
> >> On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
> >> > In order to demonstrate and test a MiscDeviceRegistration with a parent
> >> > device, introduce CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT.
> >> >
> >> > If CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT=y the misc device sample
> >> > is initialized with a parent device (faux), otherwise it is initialized
> >> > without a parent device, i.e. the exact same way as without this patch.
> >> >
> >> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> >> > ---
> >> >  samples/rust/Kconfig             |  8 +++++
> >> >  samples/rust/rust_misc_device.rs | 50 +++++++++++++++++++++++++++++---
> >> >  2 files changed, 54 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> >> > index b1006ab4bc3c..9948ec0939ef 100644
> >> > --- a/samples/rust/Kconfig
> >> > +++ b/samples/rust/Kconfig
> >> > @@ -30,6 +30,14 @@ config SAMPLE_RUST_MISC_DEVICE
> >> >  
> >> >  	  If unsure, say N.
> >> >  
> >> > +config SAMPLE_RUST_MISC_DEVICE_WITH_PARENT
> >> > +	bool "Create a misc device with a parent device"
> >> > +	depends on SAMPLE_RUST_MISC_DEVICE
> >> > +	default n
> >> > +	help
> >> > +	  Say Y here if you want the misc device sample to create a misc
> >> > +	  device with a parent device.
> >> > +
> >> 
> >> Why not create a separate file? The `cfg`s might confuse newcomers
> >> looking at the sample.
> >
> > It would be a lot of duplicated code, unless we really *only* exercise the
> > device creation and registration part, which would be a bit unfortunate, given
> > that this sample is also a pretty good test.
> 
> We could separate the common parts into a single file and then
> `include!` that file from the two samples. (Or if the build system
> supports multi-file samples then just use that, but my gut feeling is
> that it doesn't)

The samples are normal modules, where we can have multiple files. But I don't
see how that helps.

`include!` works, but I'm not sure it's that much better.

Another option would be to put the `cfg` on the module!() macro itself and have
two separate module types, this way there is only a `cfg` on the two module!()
invocations.

> 
> I really would like to avoid `cfg` in samples.
> 
> >> >  config SAMPLE_RUST_PRINT
> >> >  	tristate "Printing macros"
> >> >  	help
> >> > diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> >> > index 9bf1a0f64e6e..175638d6d341 100644
> >> > --- a/samples/rust/rust_misc_device.rs
> >> > +++ b/samples/rust/rust_misc_device.rs
> >> > @@ -167,6 +167,9 @@
> >> >      uaccess::{UserSlice, UserSliceReader, UserSliceWriter},
> >> >  };
> >> >  
> >> > +#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
> >> > +use kernel::faux;
> >> > +
> >> >  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);
> >> > @@ -181,19 +184,33 @@
> >> >      license: "GPL",
> >> >  }
> >> >  
> >> > +#[cfg(not(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT))]
> >> >  #[pin_data]
> >> >  struct RustMiscDeviceModule {
> >> >      #[pin]
> >> >      _miscdev: MiscDeviceRegistration<RustMiscDevice>,
> >> >  }
> >> >  
> >> > -impl kernel::InPlaceModule for RustMiscDeviceModule {
> >> > -    fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
> >> > +#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
> >> > +struct RustMiscDeviceModule {
> >> > +    _faux: faux::Registration,
> >> > +    _miscdev: Pin<KBox<MiscDeviceRegistration<RustMiscDevice>>>,
> >> > +}
> >> > +
> >> > +impl RustMiscDeviceModule {
> >> > +    fn init() -> MiscDeviceOptions {
> >> >          pr_info!("Initializing Rust Misc Device Sample\n");
> >> >  
> >> > -        let options = MiscDeviceOptions {
> >> > +        MiscDeviceOptions {
> >> >              name: c_str!("rust-misc-device"),
> >> > -        };
> >> > +        }
> >> > +    }
> >> > +}
> >> > +
> >> > +#[cfg(not(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT))]
> >> > +impl kernel::InPlaceModule for RustMiscDeviceModule {
> >> > +    fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
> >> > +        let options = Self::init();
> >> >  
> >> >          try_pin_init!(Self {
> >> >              _miscdev <- MiscDeviceRegistration::register(
> >> > @@ -205,6 +222,31 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
> >> >      }
> >> >  }
> >> >  
> >> > +#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
> >> > +impl kernel::Module for RustMiscDeviceModule {
> >> > +    fn init(_module: &'static ThisModule) -> Result<Self> {
> >> > +        let options = Self::init();
> >> > +        let faux = faux::Registration::new(c_str!("rust-misc-device-sample"), None)?;
> >> > +
> >> > +        // For every other bus, this would be called from Driver::probe(), which would return a
> 
> Missing '`' around Driver::probe().
> 
> >> > +        // `Result<Pin<KBox<T>>>`, but faux always binds to a "dummy" driver, hence probe() is
> >> 
> >> Not clear what `T` is supposed to be, do you mean `Self`?
> >
> > From the perspective of the type implementing the corresponding Driver trait it
> > would indeed be `Self`. But I found it ambiguous to write `Self`, since I do *not*
> > mean `RustMiscDeviceModule` with `Self`.
> 
> Yeah that makes sense, I already entered into the `impl Driver` context
> :) How about we use `<T as Driver>::probe()` above and then `T` makes
> sense?

Yep, that sounds good.

> Another thing: faux devices don't have a `probe` in rust, so saying "not
> required" doesn't make much sense, right?

In Rust, faux does not have probe() indeed, but that's because it's "not
required"; I can't think of a use-case for a new driver (yet), where this isn't
just unnecessary overhead.

> >> > +        // not required.
> >> > +        let misc = KBox::pin_init(
> >> > +            MiscDeviceRegistration::register(
> >> > +                options,
> >> > +                Arc::pin_init(new_mutex!(Inner { value: 0_i32 }), GFP_KERNEL),
> >> > +                Some(faux.as_ref()),
> >> > +            ),
> >> > +            GFP_KERNEL,
> >> > +        )?;
> >> 
> >> You could also initialize this module variation in-place. (this would
> >> also require the pin-init change to reference initialized fields)
> >
> > Yes, I also thought about that. But this way is a bit closer to what things
> > would look like within a probe() callback.
> 
> Yeah then let's do that :)
> 
> ---
> Cheers,
> Benno

Re: [PATCH 5/7] rust: miscdevice: properly support device drivers

[Danilo Krummrich]

On Sat, May 31, 2025 at 10:05:28AM +0200, Benno Lossin wrote:
> On Sat May 31, 2025 at 12:17 AM CEST, Danilo Krummrich wrote:
> > On Fri, May 30, 2025 at 10:06:55PM +0200, Benno Lossin wrote:
> >> On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
> >> > -impl<T: MiscDevice> MiscDeviceRegistration<T> {
> >> > -    /// Register a misc device.
> >> > -    pub fn register(
> >> > +impl<T: MiscDevice> RawDeviceRegistration<T> {
> >> > +    fn new<'a>(
> >> >          opts: MiscDeviceOptions,
> >> > -        data: impl PinInit<T::RegistrationData, Error>,
> >> > -    ) -> impl PinInit<Self, Error> {
> >> > +        parent: Option<&'a Device<Bound>>,
> >> > +        data: &'a T::RegistrationData,
> >> > +    ) -> impl PinInit<Self, Error> + 'a
> >> > +    where
> >> > +        T: 'a,
> >> > +    {
> >> >          try_pin_init!(Self {
> >> > -            data <- Opaque::pin_init(data),
> >> > +            // INVARIANT: `Self` is always embedded in a `MiscDeviceRegistration<T>`, hence `data`
> >> > +            // is guaranteed to be valid for the entire lifetime of `Self`.
> >> > +            data: NonNull::from(data),
> >> 
> >> Both the argument in the INVARIANT comment and way this works are a bit
> >> flawed.
> >
> > Why is the argument flawed? Let's say we go with your proposal below, what would
> > the safety requirement for RawDeviceRegistration::new and the invariant of
> > RawDeviceRegistration look like? Wouldn't it be the exact same argument?
> 
> So what I write below it not really true. But the argument relies on the
> fact that `data` is pointing to a value that will stay alive for the
> duration of the existence of `self`. That should be a safety requirement
> of `new` (even if we take a reference as an argument).

Ok, that's fair -- I'll add the safety requirement.

> >> Instead, I'd recommend directly taking the `NonNull` as a
> >> parameter. Yes the function will need to be `unsafe`, but the lifetime
> >> that you're creating below only lives for `'a`, but the object might
> >> live much longer. You might still be fine, but I'd just recommend
> >> staying in raw pointer land (or in this case `NonNull`).

Re: [PATCH 6/7] rust: miscdevice: expose the parent device as &Device<Bound>

[Danilo Krummrich]

On Sat, May 31, 2025 at 10:27:44AM +0200, Benno Lossin wrote:
> On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
> > @@ -227,11 +229,21 @@ fn drop(self: Pin<&mut Self>) {
> >      }
> >  }
> >  
> > +/// The arguments passed to the file operation callbacks of a [`MiscDeviceRegistration`].
> > +pub struct MiscArgs<'a, T: MiscDevice> {
> > +    /// The [`Device`] representation of the `struct miscdevice`.
> > +    pub device: &'a Device,
> > +    /// The parent [`Device`] of [`Self::device`].
> > +    pub parent: Option<&'a Device<Bound>>,
> > +    /// The `RegistrationData` passed to [`MiscDeviceRegistration::register`].
> > +    pub data: &'a T::RegistrationData,
> 
> Here I would also just use `T`, remove the `MiscDevice` bound and then
> use `MiscArgs<'_, Self::RegistrationData>` below.

It has the disadvantage that the documentation of the `data` field above needs
to be much more vague, since we can't claim that it's the `RegistrationData`
passed to `MiscDeviceRegistration::register` anymore -- given that, I'm not sure
it's worth changing.

> > +}
> > +
> >  /// Trait implemented by the private data of an open misc device.
> >  #[vtable]
> >  pub trait MiscDevice: Sized {
> >      /// What kind of pointer should `Self` be wrapped in.
> > -    type Ptr: ForeignOwnable + Send + Sync;
> > +    type Ptr: Send + Sync;
> 
> There is no info about this change in the commit message. Why are we
> changing this? This seems a bit orthogonal to the other change, maybe do
> it in a separate patch?

It's a consequence of the implementation:

A `Ptr` instance is created in the misc device's file operations open() callback
and dropped in the fops release() callback.

Previously, this was stored in the private data pointer of the struct file that
is passed for every file operation in open().

Also note that when open is called the private data pointer in a struct file
points to the corresponding struct miscdevice.

With this patch, we keep the pointer to the struct miscdevice in the private
data pointer of struct file, but instead store the `Ptr` instance in
`RawDeviceRegistration::private`.

Subsequently, ForeignOwnable is not a required trait anymore.

We need this in order to keep access to the `RawDeviceRegistration` throughout
file operations to be able to pass the misc device's parent as &Device<Bound>
through the `MiscArgs` above.

> Also `Ptr` doesn't make much sense for the name, since now that the
> `ForeignOwnable` bound is gone, I could set this to `Self` and then have
> access to `&Self` in the callbacks.

We can't make it `Self`, it might still be some pointer type, require pin-init,
etc. So, it has to be a generic type.

But, I agree that we should not name it `Ptr`, probably should never have been
named `Ptr`, but `Data`, `Private` or similar.

> Would that also make sense to use as a general change? So don't store
> `Self::Ptr`, but `Self` directly?

I think it can't be `Self`, see above.

Re: [PATCH 7/7] rust: sample: misc: implement device driver sample

[Miguel Ojeda]

On Sat, May 31, 2025 at 10:11 AM Benno Lossin <lossin@kernel.org> wrote:
>
> We could separate the common parts into a single file and then
> `include!` that file from the two samples. (Or if the build system
> supports multi-file samples then just use that, but my gut feeling is
> that it doesn't)

It does, in the sense that you can use Rust modules (i.e. different
files). Multi-file in the sense of linking several C files + a single
Rust crate root also works (we have a sample doing that).

Reusing the same `mod` from two different crate roots should also work
(well, unless `rustc` doesn't like it for some reason, but from a
quick test it seems OK), plus games with `#[path]` and symlinks.

> I really would like to avoid `cfg` in samples.

I think the `cfg` is not the end of the world w.r.t. learning (after
all, `cfg`s are part of the kernel all the time, and it is not the
first sample). In fact, one could argue that it is a good way to show
what `cfg` can do, in a way.

But another disadvantage of `cfg` is that then one cannot have both
modules at the same time, and thus 2 builds to test both etc.

Cheers,
Miguel

Re: [PATCH 7/7] rust: sample: misc: implement device driver sample

[Benno Lossin]

On Sat May 31, 2025 at 12:29 PM CEST, Danilo Krummrich wrote:
> On Sat, May 31, 2025 at 10:11:08AM +0200, Benno Lossin wrote:
>> On Sat May 31, 2025 at 12:24 AM CEST, Danilo Krummrich wrote:
>> > On Fri, May 30, 2025 at 10:15:37PM +0200, Benno Lossin wrote:
>> >> On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
>> >> > +config SAMPLE_RUST_MISC_DEVICE_WITH_PARENT
>> >> > +	bool "Create a misc device with a parent device"
>> >> > +	depends on SAMPLE_RUST_MISC_DEVICE
>> >> > +	default n
>> >> > +	help
>> >> > +	  Say Y here if you want the misc device sample to create a misc
>> >> > +	  device with a parent device.
>> >> > +
>> >> 
>> >> Why not create a separate file? The `cfg`s might confuse newcomers
>> >> looking at the sample.
>> >
>> > It would be a lot of duplicated code, unless we really *only* exercise the
>> > device creation and registration part, which would be a bit unfortunate, given
>> > that this sample is also a pretty good test.
>> 
>> We could separate the common parts into a single file and then
>> `include!` that file from the two samples. (Or if the build system
>> supports multi-file samples then just use that, but my gut feeling is
>> that it doesn't)
>
> The samples are normal modules, where we can have multiple files. But I don't
> see how that helps.
>
> `include!` works, but I'm not sure it's that much better.
>
> Another option would be to put the `cfg` on the module!() macro itself and have
> two separate module types, this way there is only a `cfg` on the two module!()
> invocations.

How about we do it like this:

We create samples/rust/rust_misc_device/{module.rs,parent.rs,common.rs}
and `module.rs`/`parent.rs` are the two entry points. Both of these
files:
* include `common.rs` using `include!` at the very top.
* define a `RustMiscDeviceModule` struct and implmement `InPlaceModule`
  for it.

The module-level docs, common imports constants, `module!` invocation &
other definitions stay in `common.rs`.

This way we can build them at the same time and have no cfgs :)

>> >> > @@ -205,6 +222,31 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
>> >> >      }
>> >> >  }
>> >> >  
>> >> > +#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
>> >> > +impl kernel::Module for RustMiscDeviceModule {
>> >> > +    fn init(_module: &'static ThisModule) -> Result<Self> {
>> >> > +        let options = Self::init();
>> >> > +        let faux = faux::Registration::new(c_str!("rust-misc-device-sample"), None)?;
>> >> > +
>> >> > +        // For every other bus, this would be called from Driver::probe(), which would return a
>> 
>> Missing '`' around Driver::probe().
>> 
>> >> > +        // `Result<Pin<KBox<T>>>`, but faux always binds to a "dummy" driver, hence probe() is
>> >> 
>> >> Not clear what `T` is supposed to be, do you mean `Self`?
>> >
>> > From the perspective of the type implementing the corresponding Driver trait it
>> > would indeed be `Self`. But I found it ambiguous to write `Self`, since I do *not*
>> > mean `RustMiscDeviceModule` with `Self`.
>> 
>> Yeah that makes sense, I already entered into the `impl Driver` context
>> :) How about we use `<T as Driver>::probe()` above and then `T` makes
>> sense?
>
> Yep, that sounds good.
>
>> Another thing: faux devices don't have a `probe` in rust, so saying "not
>> required" doesn't make much sense, right?
>
> In Rust, faux does not have probe() indeed, but that's because it's "not
> required"; I can't think of a use-case for a new driver (yet), where this isn't
> just unnecessary overhead.

I'd write something along the lines of "the faux rust abstractions do
not have a `probe`, since it's unnecessary, so we initialize the
registration here".

---
Cheers,
Benno

Re: [PATCH 6/7] rust: miscdevice: expose the parent device as &Device<Bound>

[Benno Lossin]

On Sat May 31, 2025 at 12:46 PM CEST, Danilo Krummrich wrote:
> On Sat, May 31, 2025 at 10:27:44AM +0200, Benno Lossin wrote:
>> On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
>> > @@ -227,11 +229,21 @@ fn drop(self: Pin<&mut Self>) {
>> >      }
>> >  }
>> >  
>> > +/// The arguments passed to the file operation callbacks of a [`MiscDeviceRegistration`].
>> > +pub struct MiscArgs<'a, T: MiscDevice> {
>> > +    /// The [`Device`] representation of the `struct miscdevice`.
>> > +    pub device: &'a Device,
>> > +    /// The parent [`Device`] of [`Self::device`].
>> > +    pub parent: Option<&'a Device<Bound>>,
>> > +    /// The `RegistrationData` passed to [`MiscDeviceRegistration::register`].
>> > +    pub data: &'a T::RegistrationData,
>> 
>> Here I would also just use `T`, remove the `MiscDevice` bound and then
>> use `MiscArgs<'_, Self::RegistrationData>` below.
>
> It has the disadvantage that the documentation of the `data` field above needs
> to be much more vague, since we can't claim that it's the `RegistrationData`
> passed to `MiscDeviceRegistration::register` anymore -- given that, I'm not sure
> it's worth changing.

Yeah that's not ideal... Then keep it this way.

>> > +}
>> > +
>> >  /// Trait implemented by the private data of an open misc device.
>> >  #[vtable]
>> >  pub trait MiscDevice: Sized {
>> >      /// What kind of pointer should `Self` be wrapped in.
>> > -    type Ptr: ForeignOwnable + Send + Sync;
>> > +    type Ptr: Send + Sync;
>> 
>> There is no info about this change in the commit message. Why are we
>> changing this? This seems a bit orthogonal to the other change, maybe do
>> it in a separate patch?
>
> It's a consequence of the implementation:
>
> A `Ptr` instance is created in the misc device's file operations open() callback
> and dropped in the fops release() callback.
>
> Previously, this was stored in the private data pointer of the struct file that
> is passed for every file operation in open().
>
> Also note that when open is called the private data pointer in a struct file
> points to the corresponding struct miscdevice.
>
> With this patch, we keep the pointer to the struct miscdevice in the private
> data pointer of struct file, but instead store the `Ptr` instance in
> `RawDeviceRegistration::private`.
>
> Subsequently, ForeignOwnable is not a required trait anymore.

That's true, but it also wouldn't hurt to keep it for this patch and do
the change in a separate one. Or mention it in the commit message :)

> We need this in order to keep access to the `RawDeviceRegistration` throughout
> file operations to be able to pass the misc device's parent as &Device<Bound>
> through the `MiscArgs` above.
>
>> Also `Ptr` doesn't make much sense for the name, since now that the
>> `ForeignOwnable` bound is gone, I could set this to `Self` and then have
>> access to `&Self` in the callbacks.
>
> We can't make it `Self`, it might still be some pointer type, require pin-init,
> etc. So, it has to be a generic type.

`MiscDevice::open` could return an `impl PinInit<Self, Error>` :)

> But, I agree that we should not name it `Ptr`, probably should never have been
> named `Ptr`, but `Data`, `Private` or similar.
>
>> Would that also make sense to use as a general change? So don't store
>> `Self::Ptr`, but `Self` directly?
>
> I think it can't be `Self`, see above.

The rust_misc_device example would still work if we changed this to
`Self`. Now it's not a complicated user of the API and someone might
want to store `Self` in an `Arc` and then store that as the private
data, as the MiscDevice is also referenced from somewhere else. But I
don't know if that is common or an intended use-case :)

For simple use-cases however, I think that `Self` definitely is the
right choice (as opposed to `Pin<KBox<Self>>` for example, as that has
an extra allocation :)

---
Cheers,
Benno

Re: [PATCH 7/7] rust: sample: misc: implement device driver sample

[Danilo Krummrich]

On Sat, May 31, 2025 at 02:03:05PM +0200, Benno Lossin wrote:
> On Sat May 31, 2025 at 12:29 PM CEST, Danilo Krummrich wrote:
> > On Sat, May 31, 2025 at 10:11:08AM +0200, Benno Lossin wrote:
> >> On Sat May 31, 2025 at 12:24 AM CEST, Danilo Krummrich wrote:
> >> > On Fri, May 30, 2025 at 10:15:37PM +0200, Benno Lossin wrote:
> >> >> On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
> >> >> > +config SAMPLE_RUST_MISC_DEVICE_WITH_PARENT
> >> >> > +	bool "Create a misc device with a parent device"
> >> >> > +	depends on SAMPLE_RUST_MISC_DEVICE
> >> >> > +	default n
> >> >> > +	help
> >> >> > +	  Say Y here if you want the misc device sample to create a misc
> >> >> > +	  device with a parent device.
> >> >> > +
> >> >> 
> >> >> Why not create a separate file? The `cfg`s might confuse newcomers
> >> >> looking at the sample.
> >> >
> >> > It would be a lot of duplicated code, unless we really *only* exercise the
> >> > device creation and registration part, which would be a bit unfortunate, given
> >> > that this sample is also a pretty good test.
> >> 
> >> We could separate the common parts into a single file and then
> >> `include!` that file from the two samples. (Or if the build system
> >> supports multi-file samples then just use that, but my gut feeling is
> >> that it doesn't)
> >
> > The samples are normal modules, where we can have multiple files. But I don't
> > see how that helps.
> >
> > `include!` works, but I'm not sure it's that much better.
> >
> > Another option would be to put the `cfg` on the module!() macro itself and have
> > two separate module types, this way there is only a `cfg` on the two module!()
> > invocations.
> 
> How about we do it like this:
> 
> We create samples/rust/rust_misc_device/{module.rs,parent.rs,common.rs}
> and `module.rs`/`parent.rs` are the two entry points. Both of these
> files:
> * include `common.rs` using `include!` at the very top.
> * define a `RustMiscDeviceModule` struct and implmement `InPlaceModule`
>   for it.
> 
> The module-level docs, common imports constants, `module!` invocation &
> other definitions stay in `common.rs`.
> 
> This way we can build them at the same time and have no cfgs :)

Seems reasonable to me -- let's do that then.

Re: [PATCH 6/7] rust: miscdevice: expose the parent device as &Device<Bound>

[Danilo Krummrich]

On Sat, May 31, 2025 at 02:10:08PM +0200, Benno Lossin wrote:
> On Sat May 31, 2025 at 12:46 PM CEST, Danilo Krummrich wrote:
> > On Sat, May 31, 2025 at 10:27:44AM +0200, Benno Lossin wrote:
> >> On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
> >> > @@ -227,11 +229,21 @@ fn drop(self: Pin<&mut Self>) {
> >> >      }
> >> >  }
> >> >  
> >> > +/// The arguments passed to the file operation callbacks of a [`MiscDeviceRegistration`].
> >> > +pub struct MiscArgs<'a, T: MiscDevice> {
> >> > +    /// The [`Device`] representation of the `struct miscdevice`.
> >> > +    pub device: &'a Device,
> >> > +    /// The parent [`Device`] of [`Self::device`].
> >> > +    pub parent: Option<&'a Device<Bound>>,
> >> > +    /// The `RegistrationData` passed to [`MiscDeviceRegistration::register`].
> >> > +    pub data: &'a T::RegistrationData,
> >> 
> >> Here I would also just use `T`, remove the `MiscDevice` bound and then
> >> use `MiscArgs<'_, Self::RegistrationData>` below.
> >
> > It has the disadvantage that the documentation of the `data` field above needs
> > to be much more vague, since we can't claim that it's the `RegistrationData`
> > passed to `MiscDeviceRegistration::register` anymore -- given that, I'm not sure
> > it's worth changing.
> 
> Yeah that's not ideal... Then keep it this way.
> 
> >> > +}
> >> > +
> >> >  /// Trait implemented by the private data of an open misc device.
> >> >  #[vtable]
> >> >  pub trait MiscDevice: Sized {
> >> >      /// What kind of pointer should `Self` be wrapped in.
> >> > -    type Ptr: ForeignOwnable + Send + Sync;
> >> > +    type Ptr: Send + Sync;
> >> 
> >> There is no info about this change in the commit message. Why are we
> >> changing this? This seems a bit orthogonal to the other change, maybe do
> >> it in a separate patch?
> >
> > It's a consequence of the implementation:
> >
> > A `Ptr` instance is created in the misc device's file operations open() callback
> > and dropped in the fops release() callback.
> >
> > Previously, this was stored in the private data pointer of the struct file that
> > is passed for every file operation in open().
> >
> > Also note that when open is called the private data pointer in a struct file
> > points to the corresponding struct miscdevice.
> >
> > With this patch, we keep the pointer to the struct miscdevice in the private
> > data pointer of struct file, but instead store the `Ptr` instance in
> > `RawDeviceRegistration::private`.
> >
> > Subsequently, ForeignOwnable is not a required trait anymore.
> 
> That's true, but it also wouldn't hurt to keep it for this patch and do
> the change in a separate one. Or mention it in the commit message :)
> 
> > We need this in order to keep access to the `RawDeviceRegistration` throughout
> > file operations to be able to pass the misc device's parent as &Device<Bound>
> > through the `MiscArgs` above.
> >
> >> Also `Ptr` doesn't make much sense for the name, since now that the
> >> `ForeignOwnable` bound is gone, I could set this to `Self` and then have
> >> access to `&Self` in the callbacks.
> >
> > We can't make it `Self`, it might still be some pointer type, require pin-init,
> > etc. So, it has to be a generic type.
> 
> `MiscDevice::open` could return an `impl PinInit<Self, Error>` :)
> 
> > But, I agree that we should not name it `Ptr`, probably should never have been
> > named `Ptr`, but `Data`, `Private` or similar.
> >
> >> Would that also make sense to use as a general change? So don't store
> >> `Self::Ptr`, but `Self` directly?
> >
> > I think it can't be `Self`, see above.
> 
> The rust_misc_device example would still work if we changed this to
> `Self`. Now it's not a complicated user of the API and someone might
> want to store `Self` in an `Arc` and then store that as the private
> data, as the MiscDevice is also referenced from somewhere else. But I
> don't know if that is common or an intended use-case :)
> 
> For simple use-cases however, I think that `Self` definitely is the
> right choice (as opposed to `Pin<KBox<Self>>` for example, as that has
> an extra allocation :)

The data returned by open() can be anything. It can also be some arbitrary
Arc<T> that already exists and is looked up in open(). It can also be something
new that is created within open() and requires in-place initialization.

So, if we want to change this, we could return an `impl PinInit<Self, Error>` as
you suggest above and initialize it in-place in
`RawDeviceRegistration::private`.

I agree that this is the correct thing to do, but that really sounds like a
subsequent patch.

Re: [PATCH 6/7] rust: miscdevice: expose the parent device as &Device<Bound>

[Benno Lossin]

On Sat May 31, 2025 at 2:39 PM CEST, Danilo Krummrich wrote:
> On Sat, May 31, 2025 at 02:10:08PM +0200, Benno Lossin wrote:
>> On Sat May 31, 2025 at 12:46 PM CEST, Danilo Krummrich wrote:
>> > But, I agree that we should not name it `Ptr`, probably should never have been
>> > named `Ptr`, but `Data`, `Private` or similar.
>> >
>> >> Would that also make sense to use as a general change? So don't store
>> >> `Self::Ptr`, but `Self` directly?
>> >
>> > I think it can't be `Self`, see above.
>> 
>> The rust_misc_device example would still work if we changed this to
>> `Self`. Now it's not a complicated user of the API and someone might
>> want to store `Self` in an `Arc` and then store that as the private
>> data, as the MiscDevice is also referenced from somewhere else. But I
>> don't know if that is common or an intended use-case :)
>> 
>> For simple use-cases however, I think that `Self` definitely is the
>> right choice (as opposed to `Pin<KBox<Self>>` for example, as that has
>> an extra allocation :)
>
> The data returned by open() can be anything. It can also be some arbitrary
> Arc<T> that already exists and is looked up in open(). It can also be something
> new that is created within open() and requires in-place initialization.
>
> So, if we want to change this, we could return an `impl PinInit<Self, Error>` as
> you suggest above and initialize it in-place in
> `RawDeviceRegistration::private`.
>
> I agree that this is the correct thing to do, but that really sounds like a
> subsequent patch.

Sounds good.

---
Cheers,
Benno