Device::drvdata() and driver/driver interaction (auxiliary)

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 <[email protected]>

rust/kernel/usb.rs +0 -0

index 9238b96c2185..05eed3f4f73e 100644

"""
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:

Quoted 11 lines

rust/kernel/usb.rs +1 -1

index 9238b96c2185..05eed3f4f73e 100644
@@ -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>() };
 

rust/kernel/usb.rs extern "C" fn disconnect_callback(intf: *mut bindings::usb_interface) { · line 94

Quoted 4 lines

rust/kernel/usb.rs +1 -1

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

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 <[email protected]>

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 <[email protected]>

rust/kernel/device.rs line 24

Quoted 5 lines

rust/kernel/device.rs +5 -0

+// 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.

rust/kernel/device.rs pub unsafe fn as_bound(&self) -> &Device<Bound> { · line 207

Quoted 8 lines

/// 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

rust/kernel/device.rs +1 -0

@@ -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

Quoted 2 lines · Alice Ryhl

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.

Quoted 10 lines

Reviewed-by Alice Ryhl <[email protected]>

Review

Quoted 5 lines

(quoted diff) +5 -0

+// 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.

Quoted 13 lines

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

Review pub unsafe fn as_bound(&self) -> &Device<Bound> { · line 207

Quoted 8 lines

/// 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

(quoted diff) +1 -0

@@ -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?

Quoted 17 lines · Danilo Krummrich

Quoted 2 lines · Alice Ryhl

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.

Quoted 10 lines

Reviewed-by Alice Ryhl <[email protected]>

Review

Quoted 5 lines

(quoted diff) +5 -0

+// 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.

Quoted 15 lines

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

Quoted 13 lines

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

rust/kernel/device.rs pub unsafe fn as_bound(&self) -> &Device<Bound> { · line 207

Quoted 8 lines

/// 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

rust/kernel/device.rs +1 -0

@@ -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().

rust/kernel/device.rs +13 -9

index 36c6eec0ceab..1a307be953c2 100644
@@ -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);
+        }
+

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.

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 <[email protected]>

Quoted 3 lines · Alice Ryhl

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. :)

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