Device: generic accessors for drvdata + Driver::unbind()

Introduce an internal device context, which is semantically equivalent to the Core device context, but reserved for bus abstractions.

This allows implementing methods for the Device type, which are limited to be used within the core context of bus abstractions, i.e. restrict the availability for drivers.

Signed-off-by Danilo Krummrich <[email protected]>

---
 rust/kernel/device.rs | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

rust/kernel/device.rs +0 -0

index 665f5ceadecc..e9094d8322d5 100644

Quoted 20 lines · Danilo Krummrich

Introduce an internal device context, which is semantically equivalent to the Core device context, but reserved for bus abstractions.

This allows implementing methods for the Device type, which are limited to be used within the core context of bus abstractions, i.e. restrict the availability for drivers.

Signed-off-by Danilo Krummrich <[email protected]>

---
 rust/kernel/device.rs | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

rust/kernel/device.rs +0 -0

index 665f5ceadecc..e9094d8322d5 100644
@@ -261,6 +261,10 @@ pub trait DeviceContext: private::Sealed {}
 /// any of the bus callbacks, such as `probe()`.
 pub struct Core;
 

rust/kernel/device.rs pub trait DeviceContext: private::Sealed {} · line 266

Quoted 3 lines

rust/kernel/device.rs +3 -0

+/// Semantically the same as [`Core`] but reserved for internal usage of the corresponding bus
+/// abstraction.
+pub struct Internal;

Naming is hard :)

As this is ONLY for the bus code to touch, why not call it Bus_Internal?

And can a driver touch this, or only the bus owner?

Quoted 27 lines · Greg KH

Quoted 17 lines · Danilo Krummrich

Introduce an internal device context, which is semantically equivalent to the Core device context, but reserved for bus abstractions.

This allows implementing methods for the Device type, which are limited to be used within the core context of bus abstractions, i.e. restrict the availability for drivers.

Signed-off-by Danilo Krummrich <[email protected]>

---
 rust/kernel/device.rs | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

rust/kernel/device.rs +0 -0

index 665f5ceadecc..e9094d8322d5 100644

rust/kernel/device.rs pub trait DeviceContext: private::Sealed {} · line 266

Quoted 6 lines

rust/kernel/device.rs +3 -0

@@ -261,6 +261,10 @@ pub trait DeviceContext: private::Sealed {}
 /// any of the bus callbacks, such as `probe()`.
 pub struct Core;
 
+/// Semantically the same as [`Core`] but reserved for internal usage of the corresponding bus
+/// abstraction.
+pub struct Internal;

Naming is hard :)

Quoted 1 line

As this is ONLY for the bus code to touch, why not call it Bus_Internal?

BusInternal is better indeed!

Quoted 1 line

And can a driver touch this, or only the bus owner?

It is to prevent drivers from getting access to functions implemented for &Device<BusInternal>.

Quoted 28 lines · Danilo Krummrich

Quoted 27 lines · Greg KH

Quoted 17 lines · Danilo Krummrich

Introduce an internal device context, which is semantically equivalent to the Core device context, but reserved for bus abstractions.

This allows implementing methods for the Device type, which are limited to be used within the core context of bus abstractions, i.e. restrict the availability for drivers.

Signed-off-by Danilo Krummrich <[email protected]>

---
 rust/kernel/device.rs | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

rust/kernel/device.rs +0 -0

index 665f5ceadecc..e9094d8322d5 100644

rust/kernel/device.rs pub trait DeviceContext: private::Sealed {} · line 266

Quoted 6 lines

rust/kernel/device.rs +3 -0

@@ -261,6 +261,10 @@ pub trait DeviceContext: private::Sealed {}
 /// any of the bus callbacks, such as `probe()`.
 pub struct Core;
 
+/// Semantically the same as [`Core`] but reserved for internal usage of the corresponding bus
+/// abstraction.
+pub struct Internal;

Naming is hard :)

Quoted 3 lines

Quoted 1 line

As this is ONLY for the bus code to touch, why not call it Bus_Internal?

BusInternal is better indeed!

I now remember that I first wanted to go for CoreInternal, but then went for just Internal, since it thought it was unnecessary to be more specific. But I now think CoreInternal would have been the correct pick.

Implement generic accessors for the private data of a driver bound to a device.

Those accessors should be used by bus abstractions from their corresponding core callbacks, such as probe(), remove(), etc.

Implementing them for device::Internal guarantees that driver's can't interfere with the logic implemented by the bus abstraction.

Signed-off-by Danilo Krummrich <[email protected]>

---
 rust/helpers/device.c | 10 ++++++++++
 rust/kernel/device.rs | 43 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 52 insertions(+), 1 deletion(-)

rust/helpers/device.c +10 -0

index b2135c6686b0..9bf252649c75 100644
@@ -8,3 +8,13 @@ int rust_helper_devm_add_action(struct device *dev,
 {
 	return devm_add_action(dev, action, data);
 }
+
+void *rust_helper_dev_get_drvdata(const struct device *dev)
+{
+	return dev_get_drvdata(dev);
+}
+
+void rust_helper_dev_set_drvdata(struct device *dev, void *data)
+{
+	dev_set_drvdata(dev, data);
+}

rust/kernel/device.rs +1 -1

index e9094d8322d5..146eba147d2f 100644
@@ -6,7 +6,7 @@
 
 use crate::{
     bindings,
-    types::{ARef, Opaque},
+    types::{ARef, ForeignOwnable, Opaque},
 };
 use core::{fmt, marker::PhantomData, ptr};
 

rust/kernel/device.rs +24 -0

+
+    /// Take ownership of the private data stored in this [`Device`].
+    ///
+    /// # Safety
+    ///
+    /// - 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 {
+        // 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 safety requirements of this function, `ptr` comes from a previous call to
+        // `into_foreign()`.
+        unsafe { T::from_foreign(ptr.cast()) }
+    }
+
+    /// 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

rust/kernel/device.rs pub unsafe fn get_device(ptr: *mut bindings::device) -> ARef<Self> { · line 70

Quoted 6 lines · Danilo Krummrich

rust/kernel/device.rs +6 -0

+impl Device<Internal> {
+    /// Store a pointer to the bound driver's private data.
+    pub fn set_drvdata(&self, data: impl ForeignOwnable) {
+        // 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()) }
+    }

Ah, but a driver's private data in the device is NOT a bus-specific thing, it's a driver-specific thing, so your previous patch about Internal being there for busses feels odd.

Quoted 28 lines

rust/kernel/device.rs +28 -0

+
+    /// Take ownership of the private data stored in this [`Device`].
+    ///
+    /// # Safety
+    ///
+    /// - 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 {
+        // 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 safety requirements of this function, `ptr` comes from a previous call to
+        // `into_foreign()`.
+        unsafe { T::from_foreign(ptr.cast()) }
+    }
+
+    /// 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`].
+    pub unsafe fn drvdata_borrow<T: ForeignOwnable>(&self) -> T::Borrowed<'_> {
+        // 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()) };

rust/kernel/device.rs pub unsafe fn get_device(ptr: *mut bindings::device) -> ARef<Self> { · line 104

Quoted 6 lines

rust/kernel/device.rs +6 -0

+
+        // SAFETY: By the safety requirements of this function, `ptr` comes from a previous call to
+        // `into_foreign()`.
+        unsafe { T::borrow(ptr.cast()) }
+    }
+}

Why can't this be part of Core?

Quoted 9 lines · Greg KH

rust/kernel/device.rs pub unsafe fn get_device(ptr: *mut bindings::device) -> ARef<Self> { · line 70

Quoted 6 lines · Danilo Krummrich

rust/kernel/device.rs +6 -0

+impl Device<Internal> {
+    /// Store a pointer to the bound driver's private data.
+    pub fn set_drvdata(&self, data: impl ForeignOwnable) {
+        // 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()) }
+    }

Quoted 3 lines

Ah, but a driver's private data in the device is NOT a bus-specific thing, it's a driver-specific thing, so your previous patch about Internal being there for busses feels odd.

It's because we only want to allow the bus abstraction to call Device::set_drvdata().

The reason is the lifecycle of the driver's private data:

It starts when the driver returns the private data object in probe(). In the bus abstraction's probe() function, we're calling set_drvdata().

At this point the ownership of the object technically goes to the device. And it is our responsibility to extract the object from dev->driver_data at some point again through drvdata_obtain(). With calling drvdata_obtain() we take back ownership of the object.

Obviously, we do this in the bus abstraction's remove() callback, where we then let the object go out of scope, such that it's destructor gets called.

In contrast, drvdata_borrow() does what its name implies, it only borrows the object from dev->driver_data, such that we can provide it for the driver to use.

In the bus abstraction's remove() callback, drvdata_obtain() must be able to proof that the object we extract from dev->driver_data is the exact object that we set when calling set_drvdata() from probe().

If we'd allow the driver to call set_drvdata() itself (which is unnecessary anyways), drivers could:

1) Call set_drvdata() multiple times, where every previous call would leak the object, since the pointer would be overwritten.

2) We'd loose any guarantee about the type we extract from dev->driver_data in the bus abstraction's remove() callback wioth drvdata_obtain().

Quoted 28 lines

Quoted 28 lines

rust/kernel/device.rs +28 -0

+
+    /// Take ownership of the private data stored in this [`Device`].
+    ///
+    /// # Safety
+    ///
+    /// - 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 {
+        // 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 safety requirements of this function, `ptr` comes from a previous call to
+        // `into_foreign()`.
+        unsafe { T::from_foreign(ptr.cast()) }
+    }
+
+    /// 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`].
+    pub unsafe fn drvdata_borrow<T: ForeignOwnable>(&self) -> T::Borrowed<'_> {
+        // 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()) };

Quoted 8 lines

rust/kernel/device.rs pub unsafe fn get_device(ptr: *mut bindings::device) -> ARef<Self> { · line 104

Quoted 6 lines

rust/kernel/device.rs +6 -0

+
+        // SAFETY: By the safety requirements of this function, `ptr` comes from a previous call to
+        // `into_foreign()`.
+        unsafe { T::borrow(ptr.cast()) }
+    }
+}

Why can't this be part of Core?

Device::drvdata_borrow() itself can indeed be part of Core. It has to remain unsafe though, because the type T has to match the type that the driver returned from probe().

Instead, we should provide a reference of the driver's private data in every bus callback, such that drivers don't need unsafe code.

In order to not tempt drivers to use the unsafe method drvdata_borrow() directly, I went for hiding it behind the BusInternal device context.

rust/kernel/device.rs +10 -0

+impl Device<Internal> {
+    /// Store a pointer to the bound driver's private data.
+    pub fn set_drvdata(&self, data: impl ForeignOwnable) {
+        // 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()) }
+    }
+
+    /// Take ownership of the private data stored in this [`Device`].
+    ///
+    /// # Safety

rust/kernel/device.rs +21 -0

+        unsafe { T::from_foreign(ptr.cast()) }
+    }
+
+    /// 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`].
+    pub unsafe fn drvdata_borrow<T: ForeignOwnable>(&self) -> T::Borrowed<'_> {
+        // 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 safety requirements of this function, `ptr` comes from a previous call to
+        // `into_foreign()`.
+        unsafe { T::borrow(ptr.cast()) }
+    }
+}
+
 impl<Ctx: DeviceContext> Device<Ctx> {
     /// Obtain the raw `struct device *`.
     pub(crate) fn as_raw(&self) -> *mut bindings::device {

Quoted 10 lines · Danilo Krummrich

(quoted diff) +10 -0

+impl Device<Internal> {
+    /// Store a pointer to the bound driver's private data.
+    pub fn set_drvdata(&self, data: impl ForeignOwnable) {
+        // 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()) }
+    }
+
+    /// Take ownership of the private data stored in this [`Device`].
+    ///
+    /// # Safety

Review

Quoted 10 lines

(quoted diff) +10 -0

+    ///
+    /// - 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 {
+        // 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 safety requirements of this function, `ptr` comes from a previous call to
+        // `into_foreign()`.

Well, you're also relying on dev_get_drvdata to return the same pointer that was given to dev_set_drvdata.

Otherwise the safety docs look fine.

Quoted 25 lines · Benno Lossin

Quoted 10 lines · Danilo Krummrich

(quoted diff) +10 -0

+impl Device<Internal> {
+    /// Store a pointer to the bound driver's private data.
+    pub fn set_drvdata(&self, data: impl ForeignOwnable) {
+        // 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()) }
+    }
+
+    /// Take ownership of the private data stored in this [`Device`].
+    ///
+    /// # Safety

Review

Quoted 10 lines

(quoted diff) +10 -0

+    ///
+    /// - 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 {
+        // 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 safety requirements of this function, `ptr` comes from a previous call to
+        // `into_foreign()`.

Well, you're also relying on dev_get_drvdata to return the same pointer that was given to dev_set_drvdata.

Quoted 1 line

Otherwise the safety docs look fine.

Great! What do you think about:

rust/kernel/device.rs +0 -0

index 146eba147d2f..b01cb8e8dab3 100644

rust/kernel/device.rs +0 -0

         unsafe { T::from_foreign(ptr.cast()) }
     }

rust/kernel/device.rs +30 -0

+impl Device<Internal> {
+    /// Store a pointer to the bound driver's private data.
+    pub fn set_drvdata(&self, data: impl ForeignOwnable) {
+        // 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()) }
+    }
+
+    /// Take ownership of the private data stored in this [`Device`].
+    ///
+    /// # Safety
+    ///
+    /// - 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 {
+        // 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 safety requirements of this function, `ptr` comes from a previous call to
+        // `into_foreign()`.
+        unsafe { T::from_foreign(ptr.cast()) }
+    }
+
+    /// 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

    let pdev = unsafe { &*pdev.cast::<Device<InternalSet<Pin<KBox<T>>>>>() };
    ...
    // The type of the driver data is already known from `pdev`'s type,
    // so this can be safe.
    let foo = pdev.as_ref().drvdata_borrow();

I don't see any use of drvdata_borrow in this patchset, so I cannot really assess the benefit of making it safe, but for your consideration. ^_^;

Take advantage of the generic drvdata accessors of the generic Device type.

While at it, use from_result() instead of match.

Signed-off-by Danilo Krummrich <[email protected]>

---
 rust/helpers/pci.c | 10 ----------
 rust/kernel/pci.rs | 31 ++++++++++++++-----------------
 2 files changed, 14 insertions(+), 27 deletions(-)

rust/helpers/pci.c +0 -10

index cd0e6bf2cc4d..ef9cb38c81a6 100644
@@ -2,16 +2,6 @@
 
 #include <linux/pci.h>
 
-void rust_helper_pci_set_drvdata(struct pci_dev *pdev, void *data)
-{
-	pci_set_drvdata(pdev, data);
-}
-
-void *rust_helper_pci_get_drvdata(struct pci_dev *pdev)
-{
-	return pci_get_drvdata(pdev);
-}
-
 resource_size_t rust_helper_pci_resource_len(struct pci_dev *pdev, int bar)
 {
 	return pci_resource_len(pdev, bar);

rust/kernel/pci.rs +14 -17

index 8435f8132e38..064e74a90904 100644
@@ -10,11 +10,11 @@
     device_id::RawDeviceId,
     devres::Devres,
     driver,
-    error::{to_result, Result},
+    error::{from_result, to_result, Result},
     io::Io,
     io::IoRaw,
     str::CStr,
-    types::{ARef, ForeignOwnable, Opaque},
+    types::{ARef, Opaque},
     ThisModule,
 };
 use core::{

@@ -66,35 +66,32 @@ extern "C" fn probe_callback(
         // `struct pci_dev`.
         //
         // INVARIANT: `pdev` is valid for the duration of `probe_callback()`.
-        let pdev = unsafe { &*pdev.cast::<Device<device::Core>>() };
+        let pdev = unsafe { &*pdev.cast::<Device<device::Internal>>() };
 
         // SAFETY: `DeviceId` is a `#[repr(transparent)` wrapper of `struct pci_device_id` and
         // does not add additional invariants, so it's safe to transmute.
         let id = unsafe { &*id.cast::<DeviceId>() };
         let info = T::ID_TABLE.info(id.index());
 
-        match T::probe(pdev, info) {
-            Ok(data) => {
-                // Let the `struct pci_dev` own a reference of the driver's private data.
-                // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
-                // `struct pci_dev`.
-                unsafe { bindings::pci_set_drvdata(pdev.as_raw(), data.into_foreign() as _) };
-            }
-            Err(err) => return Error::to_errno(err),
-        }
+        from_result(|| {
+            let data = T::probe(pdev, info)?;
 
-        0
+            pdev.as_ref().set_drvdata(data);
+            Ok(0)
+        })
     }
 
     extern "C" fn remove_callback(pdev: *mut bindings::pci_dev) {
         // SAFETY: The PCI bus only ever calls the remove callback with a valid pointer to a
         // `struct pci_dev`.
-        let ptr = unsafe { bindings::pci_get_drvdata(pdev) }.cast();
+        //
+        // INVARIANT: `pdev` is valid for the duration of `remove_callback()`.
+        let pdev = unsafe { &*pdev.cast::<Device<device::Internal>>() };
 
         // SAFETY: `remove_callback` is only ever called after a successful call to
-        // `probe_callback`, hence it's guaranteed that `ptr` points to a valid and initialized
-        // `KBox<T>` pointer created through `KBox::into_foreign`.
-        let _ = unsafe { KBox::<T>::from_foreign(ptr) };
+        // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
+        // and stored a `Pin<KBox<T>>`.
+        let _ = unsafe { pdev.as_ref().drvdata_obtain::<Pin<KBox<T>>>() };
     }
 }
 

Signature · 1 line

2.49.0

This patch series consists of the following three parts.

1. Introduce the 'Internal' device context (semantically identical to the 'Core' device context), but only accessible for bus abstractions.

2. Introduce generic accessors for a device's driver_data pointer. Those are implemented for the 'Internal' device context only, in order to only enable bus abstractions to mess with the driver_data pointer of struct device.

3. Implement the Driver::unbind() callback (details below).

Driver::unbind()
----------------

Currently, there's really only one core callback for drivers, which is probe().

Now, this isn't entirely true, since there is also the drop() callback of the driver type (serving as the driver's private data), which is returned by probe() and is dropped in remove().

On the C side remove() mainly serves two purposes:

(1) Tear down the device that is operated by the driver, e.g. call bus specific functions, write I/O memory to reset the device, etc.

(2) Release the resources that have been allocated by a driver for a specific device.

The drop() callback mentioned above is intended to cover (2) as the Rust idiomatic way.

However, it is partially insufficient and inefficient to cover (1) properly, since drop() can't be called with additional arguments, such as the reference to the corresponding device that has the correct device context, i.e. the Core device context.

This makes it inefficient (but not impossible) to access device resources, e.g. to write device registers, and impossible to call device methods, which are only accessible under the Core device context.

In order to solve this, add an additional callback for (1), which we call unbind().

The reason for calling it unbind() is that, unlike remove(), it is *only* meant to be used to perform teardown operations on the device (1), but *not* to release resources (2).

Quoted 32 lines · Danilo Krummrich

This patch series consists of the following three parts.

1. Introduce the 'Internal' device context (semantically identical to the 'Core' device context), but only accessible for bus abstractions.

2. Introduce generic accessors for a device's driver_data pointer. Those are implemented for the 'Internal' device context only, in order to only enable bus abstractions to mess with the driver_data pointer of struct device.

3. Implement the Driver::unbind() callback (details below).

Driver::unbind()
----------------

Currently, there's really only one core callback for drivers, which is probe().

Now, this isn't entirely true, since there is also the drop() callback of the driver type (serving as the driver's private data), which is returned by probe() and is dropped in remove().

On the C side remove() mainly serves two purposes:

(1) Tear down the device that is operated by the driver, e.g. call bus specific functions, write I/O memory to reset the device, etc.

(2) Release the resources that have been allocated by a driver for a specific device.

The drop() callback mentioned above is intended to cover (2) as the Rust idiomatic way.

Quoted 4 lines

However, it is partially insufficient and inefficient to cover (1) properly, since drop() can't be called with additional arguments, such as the reference to the corresponding device that has the correct device context, i.e. the Core device context.

I'm missing something, why doesn't drop() have access to the device itself, which has the Core device context? It's the same "object", right?

Quoted 7 lines

This makes it inefficient (but not impossible) to access device resources, e.g. to write device registers, and impossible to call device methods, which are only accessible under the Core device context.

In order to solve this, add an additional callback for (1), which we call unbind().

Quoted 3 lines

The reason for calling it unbind() is that, unlike remove(), it is *only* meant to be used to perform teardown operations on the device (1), but *not* to release resources (2).

Ick. I get the idea, but unbind() is going to get confusing fast. Determining what is, and is not, a "resource" is going to be hard over time. In fact, how would you define it? :)

Is "teardown" only allowed to write to resources, but not free them?

If so, why can't that happen in drop() as the resources are available there.

I'm loath to have a 2-step destroy system here for rust only, and not for C, as maintaining this over time is going to be rough.

Quoted 7 lines

Quoted 7 lines

This makes it inefficient (but not impossible) to access device resources, e.g. to write device registers, and impossible to call device methods, which are only accessible under the Core device context.

In order to solve this, add an additional callback for (1), which we call unbind().

Quoted 7 lines

Quoted 3 lines

The reason for calling it unbind() is that, unlike remove(), it is *only* meant to be used to perform teardown operations on the device (1), but *not* to release resources (2).

Ick. I get the idea, but unbind() is going to get confusing fast. Determining what is, and is not, a "resource" is going to be hard over time. In fact, how would you define it? :)

I think the definition is simple: All teardown operations a driver needs a &Device<Core> for go into unbind().

Whereas drop() really only is the destructor of the driver's private data.

Quoted 1 line

Is "teardown" only allowed to write to resources, but not free them?

"Teardown" is everything that involves interaction with the device when the driver is unbound.

However, we can't free things there, that happens in the automatically when the destructor of the driver's private data is called, i.e. in drop().

Quoted 25 lines · Danilo Krummrich

Quoted 7 lines

Quoted 7 lines

This makes it inefficient (but not impossible) to access device resources, e.g. to write device registers, and impossible to call device methods, which are only accessible under the Core device context.

In order to solve this, add an additional callback for (1), which we call unbind().

Quoted 7 lines

Quoted 3 lines

The reason for calling it unbind() is that, unlike remove(), it is *only* meant to be used to perform teardown operations on the device (1), but *not* to release resources (2).

Ick. I get the idea, but unbind() is going to get confusing fast. Determining what is, and is not, a "resource" is going to be hard over time. In fact, how would you define it? :)

I think the definition is simple: All teardown operations a driver needs a &Device<Core> for go into unbind().

Whereas drop() really only is the destructor of the driver's private data.

Quoted 1 line

Is "teardown" only allowed to write to resources, but not free them?

"Teardown" is everything that involves interaction with the device when the driver is unbound.

Quoted 2 lines

However, we can't free things there, that happens in the automatically when the destructor of the driver's private data is called, i.e. in drop().

Can't we somehow make a (renamed) unbind receive full ownership of the driver's private data, such that it will be freed (and its drop implementation called) before unbind returns? Or do we necessarily need to free that data later?

rust: device: introduce device::Internal

rust: device: add drvdata accessors

rust: platform: use generic device drvdata accessors
rust: pci: use generic device drvdata accessors
rust: auxiliary: use generic device drvdata accessors
rust: platform: implement Driver::unbind()
rust: pci: implement Driver::unbind()
samples: rust: pci: reset pci-testdev in unbind()