[PATCH 0/8] Device::drvdata() and driver/driver interaction (auxiliary)

[PATCH 0/8] Device::drvdata() and driver/driver interaction (auxiliary)

[Danilo Krummrich]

tl;dr:

Implement a safe Device<Bound>::drvdata() accessor (used for driver to
driver interactions) based on the auxiliary bus.

This provides a way to derive a driver's device private data when
serving as a parent in a driver hierarchy, such as a driver utilizing
the auxiliary bus.

Please have a look at patch 8 ("samples: rust: auxiliary: illustrate
driver interaction") to see how it turns out.

--

Full cover letter:

In C dev_get_drvdata() has specific requirements under which it is valid
to access the returned pointer. That is, drivers have to ensure that

  (1) for the duration the returned pointer is accessed the driver is
      bound and remains to be bound to the corresponding device,

  (2) the returned void * is treated according to the driver's private
      data type, i.e. according to what has been passed to
      dev_set_drvdata().

In Rust, (1) can be ensured by simply requiring the Bound device
context, i.e. provide the drvdata() method for Device<Bound> only.

For (2) we would usually make the device type generic over the driver
type, e.g. Device<T: Driver>, where <T as Driver>::Data is the type of
the driver's private data.

However, a device does not have a driver type known at compile time and
may be bound to multiple drivers throughout its lifetime.

Hence, in order to be able to provide a safe accessor for the driver's
device private data, we have to do the type check on runtime.

This is achieved by letting a driver assert the expected type, which is
then compared to a type hash stored in struct device_private when
dev_set_drvdata() is called [2].

Example:

        // `dev` is a `&Device<Bound>`.
        let data = dev.drvdata::<SampleDriver>()?;

There are two aspects to note:

  (1) Technically, the same check could be achieved by comparing the
      struct device_driver pointer of struct device with the struct
      device_driver pointer of the driver struct (e.g. struct
      pci_driver).

      However, this would - in addition the pointer comparison - require
      to tie back the private driver data type to the struct
      device_driver pointer of the driver struct to prove correctness.

      Besides that, accessing the driver struct (stored in the module
      structure) isn't trivial and would result into horrible code and
      API ergonomics.

  (2) Having a direct accessor to the driver's private data is not
      commonly required (at least in Rust): Bus callback methods already
      provide access to the driver's device private data through a &self
      argument, while other driver entry points such as IRQs,
      workqueues, timers, IOCTLs, etc. have their own private data with
      separate ownership and lifetime.

      In other words, a driver's device private data is only relevant
      for driver model contexts (such a file private is only relevant
      for file contexts).

Having that said, the motivation for accessing the driver's device
private data with Device<Bound>::drvdata() are interactions between
drivers. For instance, when an auxiliary driver calls back into its
parent, the parent has to be capable to derive its private data from the
corresponding device (i.e. the parent of the auxiliary device).

Therefore this patch series also contains the corresponding patches for
the auxiliary bus abstraction, i.e. guarantee that the auxiliary
device's parent is guaranteed to be bound when the auxiliary device
itself is guaranteed to be bound, plus the corresponding
Device<Bound>::parent() method.

Finally, illustrate how things turn out by updating the auxiliary sample
driver.

Similarly, the same thing can be done for PCI virtual function drivers
calling back into the corresponding physical function driver or MFD.

The former (PCI PF/VF interaction) will be addressed by a separate patch
series. Both, auxiliary and PCI PF/VF is required by the Nova project.

A branch containing the series can be found in [1].

[1] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=drvdata
[2] Type hash (TypeId) stored in struct device_private:

        The Rust type stored in struct device_private could be replaced
        by a dedicated (and transparent) private pointer (e.g.
        struct device_private::rust).

        While I'm not overly concerned about the extra allocation (not a
        hot path at all), I still wanted to try to store it directly in
        struct device_private, see how it turns out and gather opinions.

        Additionally, I don't expect any additional Rust specific
        private data to be required. But even if, changing things up to
        use a separate transparent allocation in the future is trivial.

Danilo Krummrich (8):
  rust: device: narrow the generic of drvdata_obtain()
  rust: device: introduce Device::drvdata()
  rust: auxiliary: consider auxiliary devices always have a parent
  rust: auxiliary: unregister on parent device unbind
  rust: auxiliary: move parent() to impl Device
  rust: auxiliary: implement parent() for Device<Bound>
  samples: rust: auxiliary: misc cleanup of ParentDriver::connect()
  samples: rust: auxiliary: illustrate driver interaction

 drivers/base/base.h                   |  16 ++++
 drivers/gpu/drm/nova/file.rs          |   2 +-
 drivers/gpu/nova-core/driver.rs       |   8 +-
 rust/bindings/bindings_helper.h       |   6 ++
 rust/kernel/auxiliary.rs              | 108 ++++++++++++++++----------
 rust/kernel/device.rs                 |  83 ++++++++++++++++++--
 rust/kernel/pci.rs                    |   2 +-
 rust/kernel/platform.rs               |   2 +-
 rust/kernel/usb.rs                    |   4 +-
 samples/rust/rust_driver_auxiliary.rs |  44 +++++++----
 10 files changed, 207 insertions(+), 68 deletions(-)


base-commit: b782675fc7b5ba0124e26cab935a5285278c8278
-- 
2.51.0

[PATCH 1/8] rust: device: narrow the generic of drvdata_obtain()

[Danilo Krummrich]

Let T be the actual private driver data type without the surrounding
box, as it leaves less room for potential bugs.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/auxiliary.rs | 2 +-
 rust/kernel/device.rs    | 4 ++--
 rust/kernel/pci.rs       | 2 +-
 rust/kernel/platform.rs  | 2 +-
 rust/kernel/usb.rs       | 4 ++--
 5 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs
index e12f78734606..a6a2b23befce 100644
--- a/rust/kernel/auxiliary.rs
+++ b/rust/kernel/auxiliary.rs
@@ -85,7 +85,7 @@ extern "C" fn remove_callback(adev: *mut bindings::auxiliary_device) {
         // SAFETY: `remove_callback` is only ever called after a successful call to
         // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
         // and stored a `Pin<KBox<T>>`.
-        drop(unsafe { adev.as_ref().drvdata_obtain::<Pin<KBox<T>>>() });
+        drop(unsafe { adev.as_ref().drvdata_obtain::<T>() });
     }
 }
 
diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 343996027c89..106aa57a6385 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -215,7 +215,7 @@ pub fn set_drvdata<T: 'static>(&self, data: impl PinInit<T, Error>) -> Result {
     /// - Must only be called once after a preceding call to [`Device::set_drvdata`].
     /// - The type `T` must match the type of the `ForeignOwnable` previously stored by
     ///   [`Device::set_drvdata`].
-    pub unsafe fn drvdata_obtain<T: ForeignOwnable>(&self) -> T {
+    pub unsafe fn drvdata_obtain<T: 'static>(&self) -> Pin<KBox<T>> {
         // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
         let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };
 
@@ -224,7 +224,7 @@ pub unsafe fn drvdata_obtain<T: ForeignOwnable>(&self) -> T {
         //   `into_foreign()`.
         // - `dev_get_drvdata()` guarantees to return the same pointer given to `dev_set_drvdata()`
         //   in `into_foreign()`.
-        unsafe { T::from_foreign(ptr.cast()) }
+        unsafe { Pin::<KBox<T>>::from_foreign(ptr.cast()) }
     }
 
     /// Borrow the driver's private data bound to this [`Device`].
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 83e19bcec46e..e90b13aebac8 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -148,7 +148,7 @@ extern "C" fn remove_callback(pdev: *mut bindings::pci_dev) {
         // SAFETY: `remove_callback` is only ever called after a successful call to
         // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
         // and stored a `Pin<KBox<T>>`.
-        let data = unsafe { pdev.as_ref().drvdata_obtain::<Pin<KBox<T>>>() };
+        let data = unsafe { pdev.as_ref().drvdata_obtain::<T>() };
 
         T::unbind(pdev, data.as_ref());
     }
diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index 043721fdb6d8..8f7522c4cf89 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -91,7 +91,7 @@ extern "C" fn remove_callback(pdev: *mut bindings::platform_device) {
         // SAFETY: `remove_callback` is only ever called after a successful call to
         // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
         // and stored a `Pin<KBox<T>>`.
-        let data = unsafe { pdev.as_ref().drvdata_obtain::<Pin<KBox<T>>>() };
+        let data = unsafe { pdev.as_ref().drvdata_obtain::<T>() };
 
         T::unbind(pdev, data.as_ref());
     }
diff --git a/rust/kernel/usb.rs b/rust/kernel/usb.rs
index 9238b96c2185..05eed3f4f73e 100644
--- a/rust/kernel/usb.rs
+++ b/rust/kernel/usb.rs
@@ -87,9 +87,9 @@ extern "C" fn disconnect_callback(intf: *mut bindings::usb_interface) {
         // SAFETY: `disconnect_callback` is only ever called after a successful call to
         // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
         // and stored a `Pin<KBox<T>>`.
-        let data = unsafe { dev.drvdata_obtain::<Pin<KBox<T>>>() };
+        let data = unsafe { dev.drvdata_obtain::<T>() };
 
-        T::disconnect(intf, data.as_ref());
+        T::disconnect(intf, data.data());
     }
 }
 
-- 
2.51.0

[PATCH 2/8] rust: device: introduce Device::drvdata()

[Danilo Krummrich]

In C dev_get_drvdata() has specific requirements under which it is valid
to access the returned pointer. That is, drivers have to ensure that

  (1) for the duration the returned pointer is accessed the driver is
      bound and remains to be bound to the corresponding device,

  (2) the returned void * is treated according to the driver's private
      data type, i.e. according to what has been passed to
      dev_set_drvdata().

In Rust, (1) can be ensured by simply requiring the Bound device
context, i.e. provide the drvdata() method for Device<Bound> only.

For (2) we would usually make the device type generic over the driver
type, e.g. Device<T: Driver>, where <T as Driver>::Data is the type of
the driver's private data.

However, a device does not have a driver type known at compile time and
may be bound to multiple drivers throughout its lifetime.

Hence, in order to be able to provide a safe accessor for the driver's
device private data, we have to do the type check on runtime.

This is achieved by letting a driver assert the expected type, which is
then compared to a type hash stored in struct device_private when
dev_set_drvdata() is called.

Example:

	// `dev` is a `&Device<Bound>`.
	let data = dev.drvdata::<SampleDriver>()?;

There are two aspects to note:

  (1) Technically, the same check could be achieved by comparing the
      struct device_driver pointer of struct device with the struct
      device_driver pointer of the driver struct (e.g. struct
      pci_driver).

      However, this would - in addition the pointer comparison - require
      to tie back the private driver data type to the struct
      device_driver pointer of the driver struct to prove correctness.

      Besides that, accessing the driver struct (stored in the module
      structure) isn't trivial and would result into horrible code and
      API ergonomics.

  (2) Having a direct accessor to the driver's private data is not
      commonly required (at least in Rust): Bus callback methods already
      provide access to the driver's device private data through a &self
      argument, while other driver entry points such as IRQs,
      workqueues, timers, IOCTLs, etc. have their own private data with
      separate ownership and lifetime.

      In other words, a driver's device private data is only relevant
      for driver model contexts (such a file private is only relevant
      for file contexts).

Having that said, the motivation for accessing the driver's device
private data with Device<Bound>::drvdata() are interactions between
drivers. For instance, when an auxiliary driver calls back into its
parent, the parent has to be capable to derive its private data from the
corresponding device (i.e. the parent of the auxiliary device).

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 drivers/base/base.h             | 16 +++++++
 rust/bindings/bindings_helper.h |  6 +++
 rust/kernel/device.rs           | 79 +++++++++++++++++++++++++++++++--
 3 files changed, 98 insertions(+), 3 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 86fa7fbb3548..430cbefbc97f 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -85,6 +85,18 @@ struct driver_private {
 };
 #define to_driver(obj) container_of(obj, struct driver_private, kobj)
 
+#ifdef CONFIG_RUST
+/**
+ * struct driver_type - Representation of a Rust driver type.
+ */
+struct driver_type {
+	/**
+	 * @id: Representation of core::any::TypeId.
+	 */
+	u8 id[16];
+} __packed;
+#endif
+
 /**
  * struct device_private - structure to hold the private to the driver core portions of the device structure.
  *
@@ -100,6 +112,7 @@ struct driver_private {
  * @async_driver - pointer to device driver awaiting probe via async_probe
  * @device - pointer back to the struct device that this structure is
  * associated with.
+ * @driver_type - The type of the bound Rust driver.
  * @dead - This device is currently either in the process of or has been
  *	removed from the system. Any asynchronous events scheduled for this
  *	device should exit without taking any action.
@@ -116,6 +129,9 @@ struct device_private {
 	const struct device_driver *async_driver;
 	char *deferred_probe_reason;
 	struct device *device;
+#ifdef CONFIG_RUST
+	struct driver_type driver_type;
+#endif
 	u8 dead:1;
 };
 #define to_device_private_parent(obj)	\
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 2e43c66635a2..a79fd111f886 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -85,6 +85,12 @@
 #include <linux/xarray.h>
 #include <trace/events/rust_sample.h>
 
+/*
+ * The driver-core Rust code needs to know about some C driver-core private
+ * structures.
+ */
+#include <../../drivers/base/base.h>
+
 #if defined(CONFIG_DRM_PANIC_SCREEN_QR_CODE)
 // Used by `#[export]` in `drivers/gpu/drm/drm_panic_qr.rs`.
 #include <drm/drm_panic.h>
diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 106aa57a6385..36c6eec0ceab 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -10,13 +10,19 @@
     sync::aref::ARef,
     types::{ForeignOwnable, Opaque},
 };
-use core::{marker::PhantomData, ptr};
+use core::{any::TypeId, marker::PhantomData, ptr};
 
 #[cfg(CONFIG_PRINTK)]
 use crate::c_str;
 
 pub mod property;
 
+// Compile-time checks.
+const _: () = {
+    // Assert that we can `read()` / `write()` a `TypeId` instance from / into `struct driver_type`.
+    static_assert!(core::mem::size_of::<bindings::driver_type>() == core::mem::size_of::<TypeId>());
+};
+
 /// The core representation of a device in the kernel's driver model.
 ///
 /// This structure represents the Rust abstraction for a C `struct device`. A [`Device`] can either
@@ -198,12 +204,29 @@ pub unsafe fn as_bound(&self) -> &Device<Bound> {
 }
 
 impl Device<CoreInternal> {
+    fn type_id_store<T: 'static>(&self) {
+        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
+        let private = unsafe { (*self.as_raw()).p };
+
+        // SAFETY: For a bound device (implied by the `CoreInternal` device context), `private` is
+        // guaranteed to be a valid pointer to a `struct device_private`.
+        let driver_type = unsafe { &raw mut (*private).driver_type };
+
+        // SAFETY: `driver_type` is valid for (unaligned) writes of a `TypeId`.
+        unsafe {
+            driver_type
+                .cast::<TypeId>()
+                .write_unaligned(TypeId::of::<T>())
+        };
+    }
+
     /// Store a pointer to the bound driver's private data.
     pub fn set_drvdata<T: 'static>(&self, data: impl PinInit<T, Error>) -> Result {
         let data = KBox::pin_init(data, GFP_KERNEL)?;
 
         // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
         unsafe { bindings::dev_set_drvdata(self.as_raw(), data.into_foreign().cast()) };
+        self.type_id_store::<T>();
 
         Ok(())
     }
@@ -235,7 +258,23 @@ pub unsafe fn drvdata_obtain<T: 'static>(&self) -> Pin<KBox<T>> {
     ///   [`Device::drvdata_obtain`].
     /// - The type `T` must match the type of the `ForeignOwnable` previously stored by
     ///   [`Device::set_drvdata`].
-    pub unsafe fn drvdata_borrow<T: ForeignOwnable>(&self) -> T::Borrowed<'_> {
+    pub unsafe fn drvdata_borrow<T: 'static>(&self) -> Pin<&T> {
+        // SAFETY: `drvdata_unchecked()` has the exact same safety requirements as the ones
+        // required by this method.
+        unsafe { self.drvdata_unchecked() }
+    }
+}
+
+impl Device<Bound> {
+    /// Borrow the driver's private data bound to this [`Device`].
+    ///
+    /// # Safety
+    ///
+    /// - Must only be called after a preceding call to [`Device::set_drvdata`] and before
+    ///   [`Device::drvdata_obtain`].
+    /// - The type `T` must match the type of the `ForeignOwnable` previously stored by
+    ///   [`Device::set_drvdata`].
+    unsafe fn drvdata_unchecked<T: 'static>(&self) -> Pin<&T> {
         // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
         let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };
 
@@ -244,7 +283,41 @@ pub unsafe fn drvdata_borrow<T: ForeignOwnable>(&self) -> T::Borrowed<'_> {
         //   `into_foreign()`.
         // - `dev_get_drvdata()` guarantees to return the same pointer given to `dev_set_drvdata()`
         //   in `into_foreign()`.
-        unsafe { T::borrow(ptr.cast()) }
+        unsafe { Pin::<KBox<T>>::borrow(ptr.cast()) }
+    }
+
+    fn type_id_match<T: 'static>(&self) -> Result {
+        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
+        let private = unsafe { (*self.as_raw()).p };
+
+        // SAFETY: For a bound device, `private` is guaranteed to be a valid pointer to a
+        // `struct device_private`.
+        let driver_type = unsafe { &raw mut (*private).driver_type };
+
+        // SAFETY:
+        // - `driver_type` is valid for (unaligned) reads of a `TypeId`.
+        // - A bound device guarantees that `driver_type` contains a valid `TypeId` value.
+        let type_id = unsafe { driver_type.cast::<TypeId>().read_unaligned() };
+
+        if type_id != TypeId::of::<T>() {
+            return Err(EINVAL);
+        }
+
+        Ok(())
+    }
+
+    /// Access a driver's private data.
+    ///
+    /// Returns a pinned reference to the driver's private data or [`EINVAL`] if it doesn't match
+    /// the asserted type `T`.
+    pub fn drvdata<T: 'static>(&self) -> Result<Pin<&T>> {
+        self.type_id_match::<T>()?;
+
+        // SAFETY:
+        // - The `Bound` device context guarantees that this is only ever call after a call
+        //   to `set_drvdata()` and before `drvdata_obtain()`.
+        // - We've just checked that the type of the driver's private data is in fact `T`.
+        Ok(unsafe { self.drvdata_unchecked() })
     }
 }
 
-- 
2.51.0

[PATCH 3/8] rust: auxiliary: consider auxiliary devices always have a parent

[Danilo Krummrich]

An auxiliary device is guaranteed to always have a parent device (both
in C and Rust), hence don't return an Option<&auxiliary::Device> in
auxiliary::Device::parent().

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 drivers/gpu/drm/nova/file.rs          | 2 +-
 rust/kernel/auxiliary.rs              | 7 ++++---
 samples/rust/rust_driver_auxiliary.rs | 2 +-
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/nova/file.rs b/drivers/gpu/drm/nova/file.rs
index 90b9d2d0ec4a..a3b7bd36792c 100644
--- a/drivers/gpu/drm/nova/file.rs
+++ b/drivers/gpu/drm/nova/file.rs
@@ -28,7 +28,7 @@ pub(crate) fn get_param(
         _file: &drm::File<File>,
     ) -> Result<u32> {
         let adev = &dev.adev;
-        let parent = adev.parent().ok_or(ENOENT)?;
+        let parent = adev.parent();
         let pdev: &pci::Device = parent.try_into()?;
 
         let value = match getparam.param as u32 {
diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs
index a6a2b23befce..e5bddb738d58 100644
--- a/rust/kernel/auxiliary.rs
+++ b/rust/kernel/auxiliary.rs
@@ -215,9 +215,10 @@ pub fn id(&self) -> u32 {
         unsafe { (*self.as_raw()).id }
     }
 
-    /// Returns a reference to the parent [`device::Device`], if any.
-    pub fn parent(&self) -> Option<&device::Device> {
-        self.as_ref().parent()
+    /// Returns a reference to the parent [`device::Device`].
+    pub fn parent(&self) -> &device::Device {
+        // SAFETY: A `struct auxiliary_device` always has a parent.
+        unsafe { self.as_ref().parent().unwrap_unchecked() }
     }
 }
 
diff --git a/samples/rust/rust_driver_auxiliary.rs b/samples/rust/rust_driver_auxiliary.rs
index 0e221abe4936..2e9afeb83d4f 100644
--- a/samples/rust/rust_driver_auxiliary.rs
+++ b/samples/rust/rust_driver_auxiliary.rs
@@ -68,7 +68,7 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> impl PinInit<Self, E
 
 impl ParentDriver {
     fn connect(adev: &auxiliary::Device) -> Result<()> {
-        let parent = adev.parent().ok_or(EINVAL)?;
+        let parent = adev.parent();
         let pdev: &pci::Device = parent.try_into()?;
 
         let vendor = pdev.vendor_id();
-- 
2.51.0

[PATCH 4/8] rust: auxiliary: unregister on parent device unbind

[Danilo Krummrich]

Guarantee that an auxiliary driver will be unbound before its parent is
unbound; there is no point in operating an auxiliary device whose parent
has been unbound.

In practice, this guarantee allows us to assume that for a bound
auxiliary device, also the parent device is bound.

This is useful when an auxiliary driver calls into its parent, since it
allows the parent to directly access device resources and its device
private data due to the guaranteed bound device context.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 drivers/gpu/nova-core/driver.rs       |  8 ++-
 rust/kernel/auxiliary.rs              | 89 +++++++++++++++------------
 samples/rust/rust_driver_auxiliary.rs | 17 ++---
 3 files changed, 66 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs
index a83b86199182..ca0d5f8ad54b 100644
--- a/drivers/gpu/nova-core/driver.rs
+++ b/drivers/gpu/nova-core/driver.rs
@@ -3,6 +3,7 @@
 use kernel::{
     auxiliary, c_str,
     device::Core,
+    devres::Devres,
     pci,
     pci::{Class, ClassMask, Vendor},
     prelude::*,
@@ -16,7 +17,8 @@
 pub(crate) struct NovaCore {
     #[pin]
     pub(crate) gpu: Gpu,
-    _reg: auxiliary::Registration,
+    #[pin]
+    _reg: Devres<auxiliary::Registration>,
 }
 
 const BAR0_SIZE: usize = SZ_16M;
@@ -65,12 +67,12 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> impl PinInit<Self, E
 
             Ok(try_pin_init!(Self {
                 gpu <- Gpu::new(pdev, bar.clone(), bar.access(pdev.as_ref())?),
-                _reg: auxiliary::Registration::new(
+                _reg <- auxiliary::Registration::new(
                     pdev.as_ref(),
                     c_str!("nova-drm"),
                     0, // TODO[XARR]: Once it lands, use XArray; for now we don't use the ID.
                     crate::MODULE_NAME
-                )?,
+                ),
             }))
         })
     }
diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs
index e5bddb738d58..8c0a2472c26a 100644
--- a/rust/kernel/auxiliary.rs
+++ b/rust/kernel/auxiliary.rs
@@ -7,6 +7,7 @@
 use crate::{
     bindings, container_of, device,
     device_id::{RawDeviceId, RawDeviceIdIndex},
+    devres::Devres,
     driver,
     error::{from_result, to_result, Result},
     prelude::*,
@@ -279,8 +280,8 @@ unsafe impl Sync for Device {}
 
 /// The registration of an auxiliary device.
 ///
-/// This type represents the registration of a [`struct auxiliary_device`]. When an instance of this
-/// type is dropped, its respective auxiliary device will be unregistered from the system.
+/// This type represents the registration of a [`struct auxiliary_device`]. When its parent device
+/// is unbound, the corresponding auxiliary device will be unregistered from the system.
 ///
 /// # Invariants
 ///
@@ -290,44 +291,56 @@ unsafe impl Sync for Device {}
 
 impl Registration {
     /// Create and register a new auxiliary device.
-    pub fn new(parent: &device::Device, name: &CStr, id: u32, modname: &CStr) -> Result<Self> {
-        let boxed = KBox::new(Opaque::<bindings::auxiliary_device>::zeroed(), GFP_KERNEL)?;
-        let adev = boxed.get();
+    pub fn new<'a>(
+        parent: &'a device::Device<device::Bound>,
+        name: &'a CStr,
+        id: u32,
+        modname: &'a CStr,
+    ) -> impl PinInit<Devres<Self>, Error> + 'a {
+        pin_init::pin_init_scope(move || {
+            let boxed = KBox::new(Opaque::<bindings::auxiliary_device>::zeroed(), GFP_KERNEL)?;
+            let adev = boxed.get();
+
+            // SAFETY: It's safe to set the fields of `struct auxiliary_device` on initialization.
+            unsafe {
+                (*adev).dev.parent = parent.as_raw();
+                (*adev).dev.release = Some(Device::release);
+                (*adev).name = name.as_char_ptr();
+                (*adev).id = id;
+            }
 
-        // SAFETY: It's safe to set the fields of `struct auxiliary_device` on initialization.
-        unsafe {
-            (*adev).dev.parent = parent.as_raw();
-            (*adev).dev.release = Some(Device::release);
-            (*adev).name = name.as_char_ptr();
-            (*adev).id = id;
-        }
-
-        // SAFETY: `adev` is guaranteed to be a valid pointer to a `struct auxiliary_device`,
-        // which has not been initialized yet.
-        unsafe { bindings::auxiliary_device_init(adev) };
-
-        // Now that `adev` is initialized, leak the `Box`; the corresponding memory will be freed
-        // by `Device::release` when the last reference to the `struct auxiliary_device` is dropped.
-        let _ = KBox::into_raw(boxed);
-
-        // SAFETY:
-        // - `adev` is guaranteed to be a valid pointer to a `struct auxiliary_device`, which has
-        //   been initialialized,
-        // - `modname.as_char_ptr()` is a NULL terminated string.
-        let ret = unsafe { bindings::__auxiliary_device_add(adev, modname.as_char_ptr()) };
-        if ret != 0 {
             // SAFETY: `adev` is guaranteed to be a valid pointer to a `struct auxiliary_device`,
-            // which has been initialialized.
-            unsafe { bindings::auxiliary_device_uninit(adev) };
-
-            return Err(Error::from_errno(ret));
-        }
-
-        // SAFETY: `adev` is guaranteed to be non-null, since the `KBox` was allocated successfully.
-        //
-        // INVARIANT: The device will remain registered until `auxiliary_device_delete()` is called,
-        // which happens in `Self::drop()`.
-        Ok(Self(unsafe { NonNull::new_unchecked(adev) }))
+            // which has not been initialized yet.
+            unsafe { bindings::auxiliary_device_init(adev) };
+
+            // Now that `adev` is initialized, leak the `Box`; the corresponding memory will be
+            // freed by `Device::release` when the last reference to the `struct auxiliary_device`
+            // is dropped.
+            let _ = KBox::into_raw(boxed);
+
+            // SAFETY:
+            // - `adev` is guaranteed to be a valid pointer to a `struct auxiliary_device`, which
+            //   has been initialized,
+            // - `modname.as_char_ptr()` is a NULL terminated string.
+            let ret = unsafe { bindings::__auxiliary_device_add(adev, modname.as_char_ptr()) };
+            if ret != 0 {
+                // SAFETY: `adev` is guaranteed to be a valid pointer to a
+                // `struct auxiliary_device`, which has been initialized.
+                unsafe { bindings::auxiliary_device_uninit(adev) };
+
+                return Err(Error::from_errno(ret));
+            }
+
+            // SAFETY: `adev` is guaranteed to be non-null, since the `KBox` was allocated
+            // successfully.
+            //
+            // INVARIANT: The device will remain registered until `auxiliary_device_delete()` is
+            // called, which happens in `Self::drop()`.
+            Ok(Devres::new(
+                parent,
+                Self(unsafe { NonNull::new_unchecked(adev) }),
+            ))
+        })
     }
 }
 
diff --git a/samples/rust/rust_driver_auxiliary.rs b/samples/rust/rust_driver_auxiliary.rs
index 2e9afeb83d4f..95c552ee9489 100644
--- a/samples/rust/rust_driver_auxiliary.rs
+++ b/samples/rust/rust_driver_auxiliary.rs
@@ -5,7 +5,8 @@
 //! To make this driver probe, QEMU must be run with `-device pci-testdev`.
 
 use kernel::{
-    auxiliary, c_str, device::Core, driver, error::Error, pci, prelude::*, InPlaceModule,
+    auxiliary, c_str, device::Core, devres::Devres, driver, error::Error, pci, prelude::*,
+    InPlaceModule,
 };
 
 use pin_init::PinInit;
@@ -40,8 +41,12 @@ fn probe(adev: &auxiliary::Device<Core>, _info: &Self::IdInfo) -> impl PinInit<S
     }
 }
 
+#[pin_data]
 struct ParentDriver {
-    _reg: [auxiliary::Registration; 2],
+    #[pin]
+    _reg0: Devres<auxiliary::Registration>,
+    #[pin]
+    _reg1: Devres<auxiliary::Registration>,
 }
 
 kernel::pci_device_table!(
@@ -57,11 +62,9 @@ impl pci::Driver for ParentDriver {
     const ID_TABLE: pci::IdTable<Self::IdInfo> = &PCI_TABLE;
 
     fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> impl PinInit<Self, Error> {
-        Ok(Self {
-            _reg: [
-                auxiliary::Registration::new(pdev.as_ref(), AUXILIARY_NAME, 0, MODULE_NAME)?,
-                auxiliary::Registration::new(pdev.as_ref(), AUXILIARY_NAME, 1, MODULE_NAME)?,
-            ],
+        try_pin_init!(Self {
+            _reg0 <- auxiliary::Registration::new(pdev.as_ref(), AUXILIARY_NAME, 0, MODULE_NAME),
+            _reg1 <- auxiliary::Registration::new(pdev.as_ref(), AUXILIARY_NAME, 1, MODULE_NAME),
         })
     }
 }
-- 
2.51.0

[PATCH 5/8] rust: auxiliary: move parent() to impl Device

[Danilo Krummrich]

Currently, the parent method is implemented for any Device<Ctx>, i.e.
any device context and returns a &device::Device<Normal>.

However, a subsequent patch will introduce

	impl Device<Bound> {
	    pub fn parent() -> device::Device<Bound> { ... }
	}

which takes advantage of the fact that if the auxiliary device is bound
the parent is guaranteed to be bound as well.

I.e. the behavior we want is that all device contexts that dereference
to Bound, will use the implementation above, whereas the old
implementation should only be implemented for Device<Normal>.

Hence, move the current implementation.

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

diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs
index 8c0a2472c26a..497601f7473b 100644
--- a/rust/kernel/auxiliary.rs
+++ b/rust/kernel/auxiliary.rs
@@ -215,15 +215,15 @@ pub fn id(&self) -> u32 {
         // `struct auxiliary_device`.
         unsafe { (*self.as_raw()).id }
     }
+}
 
+impl Device {
     /// Returns a reference to the parent [`device::Device`].
     pub fn parent(&self) -> &device::Device {
         // SAFETY: A `struct auxiliary_device` always has a parent.
         unsafe { self.as_ref().parent().unwrap_unchecked() }
     }
-}
 
-impl Device {
     extern "C" fn release(dev: *mut bindings::device) {
         // SAFETY: By the type invariant `self.0.as_raw` is a pointer to the `struct device`
         // embedded in `struct auxiliary_device`.
-- 
2.51.0

[PATCH 6/8] rust: auxiliary: implement parent() for Device<Bound>

[Danilo Krummrich]

Take advantage of the fact that if the auxiliary device is bound the
parent is guaranteed to be bound as well and implement a separate
parent() method for auxiliary::Device<Bound>.

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

diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs
index 497601f7473b..cc67fa5ddde3 100644
--- a/rust/kernel/auxiliary.rs
+++ b/rust/kernel/auxiliary.rs
@@ -217,6 +217,16 @@ pub fn id(&self) -> u32 {
     }
 }
 
+impl Device<device::Bound> {
+    /// Returns a bound reference to the parent [`device::Device`].
+    pub fn parent(&self) -> &device::Device<device::Bound> {
+        let parent = (**self).parent();
+
+        // SAFETY: A bound auxiliary device always has a bound parent device.
+        unsafe { parent.as_bound() }
+    }
+}
+
 impl Device {
     /// Returns a reference to the parent [`device::Device`].
     pub fn parent(&self) -> &device::Device {
-- 
2.51.0

[PATCH 7/8] samples: rust: auxiliary: misc cleanup of ParentDriver::connect()

[Danilo Krummrich]

In ParentDriver::connect() rename parent to dev, use it for the
dev_info!() call, call pdev.vendor_() directly in the print statement
and remove the unnecessary generic type of Result.

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

diff --git a/samples/rust/rust_driver_auxiliary.rs b/samples/rust/rust_driver_auxiliary.rs
index 95c552ee9489..a5d67d4d9e83 100644
--- a/samples/rust/rust_driver_auxiliary.rs
+++ b/samples/rust/rust_driver_auxiliary.rs
@@ -70,16 +70,15 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> impl PinInit<Self, E
 }
 
 impl ParentDriver {
-    fn connect(adev: &auxiliary::Device) -> Result<()> {
-        let parent = adev.parent();
-        let pdev: &pci::Device = parent.try_into()?;
+    fn connect(adev: &auxiliary::Device) -> Result {
+        let dev = adev.parent();
+        let pdev: &pci::Device = dev.try_into()?;
 
-        let vendor = pdev.vendor_id();
         dev_info!(
-            adev.as_ref(),
+            dev,
             "Connect auxiliary {} with parent: VendorID={}, DeviceID={:#x}\n",
             adev.id(),
-            vendor,
+            pdev.vendor_id(),
             pdev.device_id()
         );
 
-- 
2.51.0

[PATCH 8/8] samples: rust: auxiliary: illustrate driver interaction

[Danilo Krummrich]

Illustrate how a parent driver of an auxiliary driver can take advantage
of the device context guarantees given by the auxiliary bus and
subsequently safely derive its device private data.

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

diff --git a/samples/rust/rust_driver_auxiliary.rs b/samples/rust/rust_driver_auxiliary.rs
index a5d67d4d9e83..5761ea314f44 100644
--- a/samples/rust/rust_driver_auxiliary.rs
+++ b/samples/rust/rust_driver_auxiliary.rs
@@ -5,10 +5,17 @@
 //! To make this driver probe, QEMU must be run with `-device pci-testdev`.
 
 use kernel::{
-    auxiliary, c_str, device::Core, devres::Devres, driver, error::Error, pci, prelude::*,
+    auxiliary, c_str,
+    device::{Bound, Core},
+    devres::Devres,
+    driver,
+    error::Error,
+    pci,
+    prelude::*,
     InPlaceModule,
 };
 
+use core::any::TypeId;
 use pin_init::PinInit;
 
 const MODULE_NAME: &CStr = <LocalModule as kernel::ModuleMetadata>::NAME;
@@ -43,6 +50,7 @@ fn probe(adev: &auxiliary::Device<Core>, _info: &Self::IdInfo) -> impl PinInit<S
 
 #[pin_data]
 struct ParentDriver {
+    private: TypeId,
     #[pin]
     _reg0: Devres<auxiliary::Registration>,
     #[pin]
@@ -63,6 +71,7 @@ impl pci::Driver for ParentDriver {
 
     fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> impl PinInit<Self, Error> {
         try_pin_init!(Self {
+            private: TypeId::of::<Self>(),
             _reg0 <- auxiliary::Registration::new(pdev.as_ref(), AUXILIARY_NAME, 0, MODULE_NAME),
             _reg1 <- auxiliary::Registration::new(pdev.as_ref(), AUXILIARY_NAME, 1, MODULE_NAME),
         })
@@ -70,9 +79,10 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> impl PinInit<Self, E
 }
 
 impl ParentDriver {
-    fn connect(adev: &auxiliary::Device) -> Result {
+    fn connect(adev: &auxiliary::Device<Bound>) -> Result {
         let dev = adev.parent();
-        let pdev: &pci::Device = dev.try_into()?;
+        let pdev: &pci::Device<Bound> = dev.try_into()?;
+        let drvdata = dev.drvdata::<Self>()?;
 
         dev_info!(
             dev,
@@ -82,6 +92,12 @@ fn connect(adev: &auxiliary::Device) -> Result {
             pdev.device_id()
         );
 
+        dev_info!(
+            dev,
+            "We have access to the private data of {:?}.\n",
+            drvdata.private
+        );
+
         Ok(())
     }
 }
-- 
2.51.0

Re: [PATCH 0/8] Device::drvdata() and driver/driver interaction (auxiliary)

[Greg KH]

On Tue, Oct 21, 2025 at 12:34:22AM +0200, Danilo Krummrich wrote:
> tl;dr:
> 
> Implement a safe Device<Bound>::drvdata() accessor (used for driver to
> driver interactions) based on the auxiliary bus.
> 
> This provides a way to derive a driver's device private data when
> serving as a parent in a driver hierarchy, such as a driver utilizing
> the auxiliary bus.
> 
> Please have a look at patch 8 ("samples: rust: auxiliary: illustrate
> driver interaction") to see how it turns out.

It turned out much nicer than I expected, nice work!

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 2/8] rust: device: introduce Device::drvdata()

[Alice Ryhl]

On Tue, Oct 21, 2025 at 12:34:24AM +0200, Danilo Krummrich wrote:
> In C dev_get_drvdata() has specific requirements under which it is valid
> to access the returned pointer. That is, drivers have to ensure that
> 
>   (1) for the duration the returned pointer is accessed the driver is
>       bound and remains to be bound to the corresponding device,
> 
>   (2) the returned void * is treated according to the driver's private
>       data type, i.e. according to what has been passed to
>       dev_set_drvdata().
> 
> In Rust, (1) can be ensured by simply requiring the Bound device
> context, i.e. provide the drvdata() method for Device<Bound> only.
> 
> For (2) we would usually make the device type generic over the driver
> type, e.g. Device<T: Driver>, where <T as Driver>::Data is the type of
> the driver's private data.
> 
> However, a device does not have a driver type known at compile time and
> may be bound to multiple drivers throughout its lifetime.
> 
> Hence, in order to be able to provide a safe accessor for the driver's
> device private data, we have to do the type check on runtime.
> 
> This is achieved by letting a driver assert the expected type, which is
> then compared to a type hash stored in struct device_private when
> dev_set_drvdata() is called.
> 
> Example:
> 
> 	// `dev` is a `&Device<Bound>`.
> 	let data = dev.drvdata::<SampleDriver>()?;
> 
> There are two aspects to note:
> 
>   (1) Technically, the same check could be achieved by comparing the
>       struct device_driver pointer of struct device with the struct
>       device_driver pointer of the driver struct (e.g. struct
>       pci_driver).
> 
>       However, this would - in addition the pointer comparison - require
>       to tie back the private driver data type to the struct
>       device_driver pointer of the driver struct to prove correctness.
> 
>       Besides that, accessing the driver struct (stored in the module
>       structure) isn't trivial and would result into horrible code and
>       API ergonomics.
> 
>   (2) Having a direct accessor to the driver's private data is not
>       commonly required (at least in Rust): Bus callback methods already
>       provide access to the driver's device private data through a &self
>       argument, while other driver entry points such as IRQs,
>       workqueues, timers, IOCTLs, etc. have their own private data with
>       separate ownership and lifetime.
> 
>       In other words, a driver's device private data is only relevant
>       for driver model contexts (such a file private is only relevant
>       for file contexts).
> 
> Having that said, the motivation for accessing the driver's device
> private data with Device<Bound>::drvdata() are interactions between
> drivers. For instance, when an auxiliary driver calls back into its
> parent, the parent has to be capable to derive its private data from the
> corresponding device (i.e. the parent of the auxiliary device).
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Are you going to open that docs PR to the Rust compiler about the size
of TypeID that we talked about? :)

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

> +// Compile-time checks.
> +const _: () = {
> +    // Assert that we can `read()` / `write()` a `TypeId` instance from / into `struct driver_type`.
> +    static_assert!(core::mem::size_of::<bindings::driver_type>() == core::mem::size_of::<TypeId>());
> +};

You don't need the "const _: ()" part. See the definition of
static_assert! to see why.

Also, I would not require equality. The Rust team did not think that it
would ever increase in size, but it may decrease.

>  /// The core representation of a device in the kernel's driver model.
>  ///
>  /// This structure represents the Rust abstraction for a C `struct device`. A [`Device`] can either
> @@ -198,12 +204,29 @@ pub unsafe fn as_bound(&self) -> &Device<Bound> {
>  }
>  
>  impl Device<CoreInternal> {
> +    fn type_id_store<T: 'static>(&self) {

This name isn't great. How about "set_type_id()" instead?

Alice 

Re: [PATCH 0/8] Device::drvdata() and driver/driver interaction (auxiliary)

[Alice Ryhl]

On Tue, Oct 21, 2025 at 12:34:22AM +0200, Danilo Krummrich wrote:
> tl;dr:
> 
> Implement a safe Device<Bound>::drvdata() accessor (used for driver to
> driver interactions) based on the auxiliary bus.
> 
> This provides a way to derive a driver's device private data when
> serving as a parent in a driver hierarchy, such as a driver utilizing
> the auxiliary bus.
> 
> Please have a look at patch 8 ("samples: rust: auxiliary: illustrate
> driver interaction") to see how it turns out.
> 
> --
> 
> Full cover letter:
> 
> In C dev_get_drvdata() has specific requirements under which it is valid
> to access the returned pointer. That is, drivers have to ensure that
> 
>   (1) for the duration the returned pointer is accessed the driver is
>       bound and remains to be bound to the corresponding device,
> 
>   (2) the returned void * is treated according to the driver's private
>       data type, i.e. according to what has been passed to
>       dev_set_drvdata().
> 
> In Rust, (1) can be ensured by simply requiring the Bound device
> context, i.e. provide the drvdata() method for Device<Bound> only.
> 
> For (2) we would usually make the device type generic over the driver
> type, e.g. Device<T: Driver>, where <T as Driver>::Data is the type of
> the driver's private data.
> 
> However, a device does not have a driver type known at compile time and
> may be bound to multiple drivers throughout its lifetime.
> 
> Hence, in order to be able to provide a safe accessor for the driver's
> device private data, we have to do the type check on runtime.
> 
> This is achieved by letting a driver assert the expected type, which is
> then compared to a type hash stored in struct device_private when
> dev_set_drvdata() is called [2].
> 
> Example:
> 
>         // `dev` is a `&Device<Bound>`.
>         let data = dev.drvdata::<SampleDriver>()?;
> 
> There are two aspects to note:
> 
>   (1) Technically, the same check could be achieved by comparing the
>       struct device_driver pointer of struct device with the struct
>       device_driver pointer of the driver struct (e.g. struct
>       pci_driver).
> 
>       However, this would - in addition the pointer comparison - require
>       to tie back the private driver data type to the struct
>       device_driver pointer of the driver struct to prove correctness.
> 
>       Besides that, accessing the driver struct (stored in the module
>       structure) isn't trivial and would result into horrible code and
>       API ergonomics.
> 
>   (2) Having a direct accessor to the driver's private data is not
>       commonly required (at least in Rust): Bus callback methods already
>       provide access to the driver's device private data through a &self
>       argument, while other driver entry points such as IRQs,
>       workqueues, timers, IOCTLs, etc. have their own private data with
>       separate ownership and lifetime.
> 
>       In other words, a driver's device private data is only relevant
>       for driver model contexts (such a file private is only relevant
>       for file contexts).
> 
> Having that said, the motivation for accessing the driver's device
> private data with Device<Bound>::drvdata() are interactions between
> drivers. For instance, when an auxiliary driver calls back into its
> parent, the parent has to be capable to derive its private data from the
> corresponding device (i.e. the parent of the auxiliary device).
> 
> Therefore this patch series also contains the corresponding patches for
> the auxiliary bus abstraction, i.e. guarantee that the auxiliary
> device's parent is guaranteed to be bound when the auxiliary device
> itself is guaranteed to be bound, plus the corresponding
> Device<Bound>::parent() method.
> 
> Finally, illustrate how things turn out by updating the auxiliary sample
> driver.
> 
> Similarly, the same thing can be done for PCI virtual function drivers
> calling back into the corresponding physical function driver or MFD.
> 
> The former (PCI PF/VF interaction) will be addressed by a separate patch
> series. Both, auxiliary and PCI PF/VF is required by the Nova project.
> 
> A branch containing the series can be found in [1].
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=drvdata
> [2] Type hash (TypeId) stored in struct device_private:
> 
>         The Rust type stored in struct device_private could be replaced
>         by a dedicated (and transparent) private pointer (e.g.
>         struct device_private::rust).
> 
>         While I'm not overly concerned about the extra allocation (not a
>         hot path at all), I still wanted to try to store it directly in
>         struct device_private, see how it turns out and gather opinions.
> 
>         Additionally, I don't expect any additional Rust specific
>         private data to be required. But even if, changing things up to
>         use a separate transparent allocation in the future is trivial.
> 
> Danilo Krummrich (8):
>   rust: device: narrow the generic of drvdata_obtain()
>   rust: device: introduce Device::drvdata()
>   rust: auxiliary: consider auxiliary devices always have a parent
>   rust: auxiliary: unregister on parent device unbind
>   rust: auxiliary: move parent() to impl Device
>   rust: auxiliary: implement parent() for Device<Bound>
>   samples: rust: auxiliary: misc cleanup of ParentDriver::connect()
>   samples: rust: auxiliary: illustrate driver interaction
> 
>  drivers/base/base.h                   |  16 ++++
>  drivers/gpu/drm/nova/file.rs          |   2 +-
>  drivers/gpu/nova-core/driver.rs       |   8 +-
>  rust/bindings/bindings_helper.h       |   6 ++
>  rust/kernel/auxiliary.rs              | 108 ++++++++++++++++----------
>  rust/kernel/device.rs                 |  83 ++++++++++++++++++--
>  rust/kernel/pci.rs                    |   2 +-
>  rust/kernel/platform.rs               |   2 +-
>  rust/kernel/usb.rs                    |   4 +-
>  samples/rust/rust_driver_auxiliary.rs |  44 +++++++----
>  10 files changed, 207 insertions(+), 68 deletions(-)

It looks like there are some patches that add code that doesn't pass
rustfmt, which are then fixed in follow-up commits. You might want to do
a pass of rustfmt after each commit.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH 2/8] rust: device: introduce Device::drvdata()

[Danilo Krummrich]

On Wed Oct 29, 2025 at 1:59 PM CET, Alice Ryhl wrote:
> Are you going to open that docs PR to the Rust compiler about the size
> of TypeID that we talked about? :)

Yes, I will -- thanks for the reminder.

> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>
>> +// Compile-time checks.
>> +const _: () = {
>> +    // Assert that we can `read()` / `write()` a `TypeId` instance from / into `struct driver_type`.
>> +    static_assert!(core::mem::size_of::<bindings::driver_type>() == core::mem::size_of::<TypeId>());
>> +};
>
> You don't need the "const _: ()" part. See the definition of
> static_assert! to see why.

Indeed, good catch -- same for the suggestions below.

> Also, I would not require equality. The Rust team did not think that it
> would ever increase in size, but it may decrease.
>
>>  /// The core representation of a device in the kernel's driver model.
>>  ///
>>  /// This structure represents the Rust abstraction for a C `struct device`. A [`Device`] can either
>> @@ -198,12 +204,29 @@ pub unsafe fn as_bound(&self) -> &Device<Bound> {
>>  }
>>  
>>  impl Device<CoreInternal> {
>> +    fn type_id_store<T: 'static>(&self) {
>
> This name isn't great. How about "set_type_id()" instead?
>
> Alice 

Re: [PATCH 0/8] Device::drvdata() and driver/driver interaction (auxiliary)

[Danilo Krummrich]

On Wed Oct 29, 2025 at 2:03 PM CET, Alice Ryhl wrote:
> It looks like there are some patches that add code that doesn't pass
> rustfmt, which are then fixed in follow-up commits. You might want to do
> a pass of rustfmt after each commit.

Oops, thanks for catching this -- my apply scripts will do exactly that. :)

> Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH 0/8] Device::drvdata() and driver/driver interaction (auxiliary)

[Danilo Krummrich]

On Wed Oct 29, 2025 at 4:33 PM CET, Danilo Krummrich wrote:
> On Wed Oct 29, 2025 at 2:03 PM CET, Alice Ryhl wrote:
>> It looks like there are some patches that add code that doesn't pass
>> rustfmt, which are then fixed in follow-up commits. You might want to do
>> a pass of rustfmt after each commit.
>
> Oops, thanks for catching this -- my apply scripts will do exactly that. :)

Actually, all patches do pass rustfmt on my end.

I assume you did not run rustfmt yourself, but spotted something that looks odd?

Re: [PATCH 2/8] rust: device: introduce Device::drvdata()

[Danilo Krummrich]

On Wed Oct 29, 2025 at 4:30 PM CET, Danilo Krummrich wrote:
> On Wed Oct 29, 2025 at 1:59 PM CET, Alice Ryhl wrote:
>> Are you going to open that docs PR to the Rust compiler about the size
>> of TypeID that we talked about? :)
>
> Yes, I will -- thanks for the reminder.
>
>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>>
>>> +// Compile-time checks.
>>> +const _: () = {
>>> +    // Assert that we can `read()` / `write()` a `TypeId` instance from / into `struct driver_type`.
>>> +    static_assert!(core::mem::size_of::<bindings::driver_type>() == core::mem::size_of::<TypeId>());
>>> +};
>>
>> You don't need the "const _: ()" part. See the definition of
>> static_assert! to see why.
>
> Indeed, good catch -- same for the suggestions below.
>
>> Also, I would not require equality. The Rust team did not think that it
>> would ever increase in size, but it may decrease.
>>
>>>  /// The core representation of a device in the kernel's driver model.
>>>  ///
>>>  /// This structure represents the Rust abstraction for a C `struct device`. A [`Device`] can either
>>> @@ -198,12 +204,29 @@ pub unsafe fn as_bound(&self) -> &Device<Bound> {
>>>  }
>>>  
>>>  impl Device<CoreInternal> {
>>> +    fn type_id_store<T: 'static>(&self) {
>>
>> This name isn't great. How about "set_type_id()" instead?

Here's the diff, including a missing check in case someone tries to call
Device::drvdata() from probe().

diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 36c6eec0ceab..1a307be953c2 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -17,11 +17,8 @@

 pub mod property;

-// Compile-time checks.
-const _: () = {
-    // Assert that we can `read()` / `write()` a `TypeId` instance from / into `struct driver_type`.
-    static_assert!(core::mem::size_of::<bindings::driver_type>() == core::mem::size_of::<TypeId>());
-};
+// Assert that we can `read()` / `write()` a `TypeId` instance from / into `struct driver_type`.
+static_assert!(core::mem::size_of::<bindings::driver_type>() >= core::mem::size_of::<TypeId>());

 /// The core representation of a device in the kernel's driver model.
 ///
@@ -204,7 +201,7 @@ pub unsafe fn as_bound(&self) -> &Device<Bound> {
 }

 impl Device<CoreInternal> {
-    fn type_id_store<T: 'static>(&self) {
+    fn set_type_id<T: 'static>(&self) {
         // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
         let private = unsafe { (*self.as_raw()).p };

@@ -226,7 +223,7 @@ pub fn set_drvdata<T: 'static>(&self, data: impl PinInit<T, Error>) -> Result {

         // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
         unsafe { bindings::dev_set_drvdata(self.as_raw(), data.into_foreign().cast()) };
-        self.type_id_store::<T>();
+        self.set_type_id::<T>();

         Ok(())
     }
@@ -242,6 +239,9 @@ pub unsafe fn drvdata_obtain<T: 'static>(&self) -> Pin<KBox<T>> {
         // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
         let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };

+        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
+        unsafe { bindings::dev_set_drvdata(self.as_raw(), core::ptr::null_mut()) };
+
         // SAFETY:
         // - By the safety requirements of this function, `ptr` comes from a previous call to
         //   `into_foreign()`.
@@ -286,7 +286,7 @@ unsafe fn drvdata_unchecked<T: 'static>(&self) -> Pin<&T> {
         unsafe { Pin::<KBox<T>>::borrow(ptr.cast()) }
     }

-    fn type_id_match<T: 'static>(&self) -> Result {
+    fn match_type_id<T: 'static>(&self) -> Result {
         // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
         let private = unsafe { (*self.as_raw()).p };

@@ -311,11 +311,16 @@ fn type_id_match<T: 'static>(&self) -> Result {
     /// Returns a pinned reference to the driver's private data or [`EINVAL`] if it doesn't match
     /// the asserted type `T`.
     pub fn drvdata<T: 'static>(&self) -> Result<Pin<&T>> {
-        self.type_id_match::<T>()?;
+        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
+        if unsafe { bindings::dev_get_drvdata(self.as_raw()) }.is_null() {
+            return Err(ENOENT);
+        }
+
+        self.match_type_id::<T>()?;

         // SAFETY:
-        // - The `Bound` device context guarantees that this is only ever call after a call
-        //   to `set_drvdata()` and before `drvdata_obtain()`.
+        // - The above check of `dev_get_drvdata()` guarantees that we are called after
+        //   `set_drvdata()` and before `drvdata_obtain()`.
         // - We've just checked that the type of the driver's private data is in fact `T`.
         Ok(unsafe { self.drvdata_unchecked() })
     }

Re: [PATCH 2/8] rust: device: introduce Device::drvdata()

[Alice Ryhl]

On Wed, Oct 29, 2025 at 06:02:45PM +0100, Danilo Krummrich wrote:
> On Wed Oct 29, 2025 at 4:30 PM CET, Danilo Krummrich wrote:
> > On Wed Oct 29, 2025 at 1:59 PM CET, Alice Ryhl wrote:
> >> Are you going to open that docs PR to the Rust compiler about the size
> >> of TypeID that we talked about? :)
> >
> > Yes, I will -- thanks for the reminder.
> >
> >> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> >>
> >>> +// Compile-time checks.
> >>> +const _: () = {
> >>> +    // Assert that we can `read()` / `write()` a `TypeId` instance from / into `struct driver_type`.
> >>> +    static_assert!(core::mem::size_of::<bindings::driver_type>() == core::mem::size_of::<TypeId>());
> >>> +};
> >>
> >> You don't need the "const _: ()" part. See the definition of
> >> static_assert! to see why.
> >
> > Indeed, good catch -- same for the suggestions below.
> >
> >> Also, I would not require equality. The Rust team did not think that it
> >> would ever increase in size, but it may decrease.
> >>
> >>>  /// The core representation of a device in the kernel's driver model.
> >>>  ///
> >>>  /// This structure represents the Rust abstraction for a C `struct device`. A [`Device`] can either
> >>> @@ -198,12 +204,29 @@ pub unsafe fn as_bound(&self) -> &Device<Bound> {
> >>>  }
> >>>  
> >>>  impl Device<CoreInternal> {
> >>> +    fn type_id_store<T: 'static>(&self) {
> >>
> >> This name isn't great. How about "set_type_id()" instead?
> 
> Here's the diff, including a missing check in case someone tries to call
> Device::drvdata() from probe().
> 
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index 36c6eec0ceab..1a307be953c2 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -17,11 +17,8 @@
> 
>  pub mod property;
> 
> -// Compile-time checks.
> -const _: () = {
> -    // Assert that we can `read()` / `write()` a `TypeId` instance from / into `struct driver_type`.
> -    static_assert!(core::mem::size_of::<bindings::driver_type>() == core::mem::size_of::<TypeId>());
> -};
> +// Assert that we can `read()` / `write()` a `TypeId` instance from / into `struct driver_type`.
> +static_assert!(core::mem::size_of::<bindings::driver_type>() >= core::mem::size_of::<TypeId>());
> 
>  /// The core representation of a device in the kernel's driver model.
>  ///
> @@ -204,7 +201,7 @@ pub unsafe fn as_bound(&self) -> &Device<Bound> {
>  }
> 
>  impl Device<CoreInternal> {
> -    fn type_id_store<T: 'static>(&self) {
> +    fn set_type_id<T: 'static>(&self) {
>          // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
>          let private = unsafe { (*self.as_raw()).p };
> 
> @@ -226,7 +223,7 @@ pub fn set_drvdata<T: 'static>(&self, data: impl PinInit<T, Error>) -> Result {
> 
>          // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
>          unsafe { bindings::dev_set_drvdata(self.as_raw(), data.into_foreign().cast()) };
> -        self.type_id_store::<T>();
> +        self.set_type_id::<T>();
> 
>          Ok(())
>      }
> @@ -242,6 +239,9 @@ pub unsafe fn drvdata_obtain<T: 'static>(&self) -> Pin<KBox<T>> {
>          // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
>          let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };
> 
> +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> +        unsafe { bindings::dev_set_drvdata(self.as_raw(), core::ptr::null_mut()) };
> +
>          // SAFETY:
>          // - By the safety requirements of this function, `ptr` comes from a previous call to
>          //   `into_foreign()`.
> @@ -286,7 +286,7 @@ unsafe fn drvdata_unchecked<T: 'static>(&self) -> Pin<&T> {
>          unsafe { Pin::<KBox<T>>::borrow(ptr.cast()) }
>      }
> 
> -    fn type_id_match<T: 'static>(&self) -> Result {
> +    fn match_type_id<T: 'static>(&self) -> Result {
>          // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
>          let private = unsafe { (*self.as_raw()).p };
> 
> @@ -311,11 +311,16 @@ fn type_id_match<T: 'static>(&self) -> Result {
>      /// Returns a pinned reference to the driver's private data or [`EINVAL`] if it doesn't match
>      /// the asserted type `T`.
>      pub fn drvdata<T: 'static>(&self) -> Result<Pin<&T>> {
> -        self.type_id_match::<T>()?;
> +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> +        if unsafe { bindings::dev_get_drvdata(self.as_raw()) }.is_null() {
> +            return Err(ENOENT);
> +        }
> +
> +        self.match_type_id::<T>()?;
> 
>          // SAFETY:
> -        // - The `Bound` device context guarantees that this is only ever call after a call
> -        //   to `set_drvdata()` and before `drvdata_obtain()`.
> +        // - The above check of `dev_get_drvdata()` guarantees that we are called after
> +        //   `set_drvdata()` and before `drvdata_obtain()`.
>          // - We've just checked that the type of the driver's private data is in fact `T`.
>          Ok(unsafe { self.drvdata_unchecked() })
>      }
> 

this looks ok to me.

Re: [PATCH 0/8] Device::drvdata() and driver/driver interaction (auxiliary)

[Danilo Krummrich]

On Tue Oct 21, 2025 at 12:34 AM CEST, Danilo Krummrich wrote:

Applied to driver-core-testing, thanks!

> Danilo Krummrich (8):
>   rust: device: narrow the generic of drvdata_obtain()
>   rust: device: introduce Device::drvdata()

    [ * Remove unnecessary `const _: ()` block,
      * rename type_id_{store,match}() to {set,match}_type_id(),
      * assert size_of::<bindings::driver_type>() >= size_of::<TypeId>(),
      * add missing check in case Device::drvdata() is called from probe().

      - Danilo ]

>   rust: auxiliary: consider auxiliary devices always have a parent
>   rust: auxiliary: unregister on parent device unbind
>   rust: auxiliary: move parent() to impl Device
>   rust: auxiliary: implement parent() for Device<Bound>
>   samples: rust: auxiliary: misc cleanup of ParentDriver::connect()
>   samples: rust: auxiliary: illustrate driver interaction

Build error on -next in rust/kernel/usb.rs:92:34 (was: Re: [PATCH 1/8] rust: device: narrow the generic of drvdata_obtain())

[Thorsten Leemhuis]

On 10/21/25 00:34, Danilo Krummrich wrote:
> Let T be the actual private driver data type without the surrounding
> box, as it leaves less room for potential bugs.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

This patch showed up in linux-next today and I wonder if that caused my
build to break on arm64 and x86:64. The error message looked like this
during "make bzimage":

"""
error[E0599]: no method named `data` found for struct `core::pin::Pin<kbox::Box<T, Kmalloc>>` in the current scope
  --> rust/kernel/usb.rs:92:34
   |
92 |         T::disconnect(intf, data.data());
   |                                  ^^^^ method not found in `core::pin::Pin<kbox::Box<T, Kmalloc>>`

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0599`.
make[2]: *** [rust/Makefile:553: rust/kernel.o] Error 1
make[1]: *** [/builddir/build/BUILD/kernel-6.18.0-build/kernel-next-20251103/linux-6.18.0-0.0.next.20251103.436.vanilla.fc44.x86_64/Makefile:1316: prepare] Error 2
make: *** [Makefile:256: __sub-make] Error 2
"""

Full log:
https://download.copr.fedorainfracloud.org/results/@kernel-vanilla/next/fedora-rawhide-aarch64/09759703-next-next-all/builder-live.log.gz

A quick search for "T::disconnect(intf, data.data());" on lore
lead me here:

> diff --git a/rust/kernel/usb.rs b/rust/kernel/usb.rs
> index 9238b96c2185..05eed3f4f73e 100644
> --- a/rust/kernel/usb.rs
> +++ b/rust/kernel/usb.rs
> @@ -87,9 +87,9 @@ extern "C" fn disconnect_callback(intf: *mut bindings::usb_interface) {
>          // SAFETY: `disconnect_callback` is only ever called after a successful call to
>          // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
>          // and stored a `Pin<KBox<T>>`.
> -        let data = unsafe { dev.drvdata_obtain::<Pin<KBox<T>>>() };
> +        let data = unsafe { dev.drvdata_obtain::<T>() };
>  
> -        T::disconnect(intf, data.as_ref());
> +        T::disconnect(intf, data.data());
>      }
>  }


Ciao, Thorsten

Re: Build error on -next in rust/kernel/usb.rs:92:34

[Danilo Krummrich]

On 11/3/25 7:43 AM, Thorsten Leemhuis wrote:
> """
> error[E0599]: no method named `data` found for struct `core::pin::Pin<kbox::Box<T, Kmalloc>>` in the current scope
>   --> rust/kernel/usb.rs:92:34
>    |
> 92 |         T::disconnect(intf, data.data());
>    |                                  ^^^^ method not found in `core::pin::Pin<kbox::Box<T, Kmalloc>>`
> 
> error: aborting due to 1 previous error
> 
> For more information about this error, try `rustc --explain E0599`.
> make[2]: *** [rust/Makefile:553: rust/kernel.o] Error 1
> make[1]: *** [/builddir/build/BUILD/kernel-6.18.0-build/kernel-next-20251103/linux-6.18.0-0.0.next.20251103.436.vanilla.fc44.x86_64/Makefile:1316: prepare] Error 2
> make: *** [Makefile:256: __sub-make] Error 2
> """
> 
> Full log:
> https://download.copr.fedorainfracloud.org/results/@kernel-vanilla/next/fedora-rawhide-aarch64/09759703-next-next-all/builder-live.log.gz
> 
> A quick search for "T::disconnect(intf, data.data());" on lore
> lead me here:
> 
>> diff --git a/rust/kernel/usb.rs b/rust/kernel/usb.rs
>> index 9238b96c2185..05eed3f4f73e 100644
>> --- a/rust/kernel/usb.rs
>> +++ b/rust/kernel/usb.rs
>> @@ -87,9 +87,9 @@ extern "C" fn disconnect_callback(intf: *mut bindings::usb_interface) {
>>          // SAFETY: `disconnect_callback` is only ever called after a successful call to
>>          // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
>>          // and stored a `Pin<KBox<T>>`.
>> -        let data = unsafe { dev.drvdata_obtain::<Pin<KBox<T>>>() };
>> +        let data = unsafe { dev.drvdata_obtain::<T>() };
>>  
>> -        T::disconnect(intf, data.as_ref());
>> +        T::disconnect(intf, data.data());
>>      }
>>  }

This error is cause by commit 6bbaa93912bf ("rust: device: narrow the generic of
drvdata_obtain()").

It seems it slipped through, since the USB abstractions are disabled in all
trees other than the USB tree. I tested with enabling them locally, but it seems
I forgot to re-enable them after a rebase etc.

I will send a patch with the following fix:

diff --git a/rust/kernel/usb.rs b/rust/kernel/usb.rs
index 92215fdc3c6a..534e3ded5442 100644
--- a/rust/kernel/usb.rs
+++ b/rust/kernel/usb.rs
@@ -89,7 +89,7 @@ extern "C" fn disconnect_callback(intf: *mut
bindings::usb_interface) {
         // and stored a `Pin<KBox<T>>`.
         let data = unsafe { dev.drvdata_obtain::<T>() };

-        T::disconnect(intf, data.data());
+        T::disconnect(intf, data.as_ref());
     }
 }