[PATCH 0/8] Device: generic accessors for drvdata + Driver::unbind()

[PATCH 0/8] Device: generic accessors for drvdata + Driver::unbind()

[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.

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

Danilo Krummrich (8):
  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()

 rust/helpers/auxiliary.c        | 10 ------
 rust/helpers/device.c           | 10 ++++++
 rust/helpers/pci.c              | 10 ------
 rust/helpers/platform.c         | 10 ------
 rust/kernel/auxiliary.rs        | 35 +++++++++-----------
 rust/kernel/device.rs           | 57 ++++++++++++++++++++++++++++++++-
 rust/kernel/pci.rs              | 47 +++++++++++++++++----------
 rust/kernel/platform.rs         | 52 +++++++++++++++++++-----------
 samples/rust/rust_driver_pci.rs | 11 ++++++-
 9 files changed, 154 insertions(+), 88 deletions(-)


base-commit: b29929b819f35503024c6a7e6ad442f6e36c68a0
-- 
2.49.0

[PATCH 1/8] rust: device: introduce device::Internal

[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 <dakr@kernel.org>
---
 rust/kernel/device.rs | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 665f5ceadecc..e9094d8322d5 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -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;
+
 /// The [`Bound`] context is the context of a bus specific device reference when it is guaranteed to
 /// be bound for the duration of its lifetime.
 pub struct Bound;
@@ -270,11 +274,13 @@ pub trait Sealed {}
 
     impl Sealed for super::Bound {}
     impl Sealed for super::Core {}
+    impl Sealed for super::Internal {}
     impl Sealed for super::Normal {}
 }
 
 impl DeviceContext for Bound {}
 impl DeviceContext for Core {}
+impl DeviceContext for Internal {}
 impl DeviceContext for Normal {}
 
 /// # Safety
@@ -312,6 +318,13 @@ fn deref(&self) -> &Self::Target {
 #[macro_export]
 macro_rules! impl_device_context_deref {
     (unsafe { $device:ident }) => {
+        // SAFETY: This macro has the exact same safety requirement as
+        // `__impl_device_context_deref!`.
+        ::kernel::__impl_device_context_deref!(unsafe {
+            $device,
+            $crate::device::Internal => $crate::device::Core
+        });
+
         // SAFETY: This macro has the exact same safety requirement as
         // `__impl_device_context_deref!`.
         ::kernel::__impl_device_context_deref!(unsafe {
@@ -345,6 +358,7 @@ fn from(dev: &$device<$src>) -> Self {
 #[macro_export]
 macro_rules! impl_device_context_into_aref {
     ($device:tt) => {
+        ::kernel::__impl_device_context_into_aref!($crate::device::Internal, $device);
         ::kernel::__impl_device_context_into_aref!($crate::device::Core, $device);
         ::kernel::__impl_device_context_into_aref!($crate::device::Bound, $device);
     };
-- 
2.49.0

[PATCH 2/8] rust: device: add drvdata accessors

[Danilo Krummrich]

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 <dakr@kernel.org>
---
 rust/helpers/device.c | 10 ++++++++++
 rust/kernel/device.rs | 43 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/rust/helpers/device.c b/rust/helpers/device.c
index b2135c6686b0..9bf252649c75 100644
--- a/rust/helpers/device.c
+++ b/rust/helpers/device.c
@@ -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);
+}
diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index e9094d8322d5..146eba147d2f 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -6,7 +6,7 @@
 
 use crate::{
     bindings,
-    types::{ARef, Opaque},
+    types::{ARef, ForeignOwnable, Opaque},
 };
 use core::{fmt, marker::PhantomData, ptr};
 
@@ -62,6 +62,47 @@ pub unsafe fn get_device(ptr: *mut bindings::device) -> ARef<Self> {
     }
 }
 
+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
+    ///   [`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 {
-- 
2.49.0

[PATCH 3/8] rust: platform: use generic device drvdata accessors

[Danilo Krummrich]

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 <dakr@kernel.org>
---
 rust/helpers/platform.c | 10 ----------
 rust/kernel/platform.rs | 36 +++++++++++++++++-------------------
 2 files changed, 17 insertions(+), 29 deletions(-)

diff --git a/rust/helpers/platform.c b/rust/helpers/platform.c
index 82171233d12f..1ce89c1a36f7 100644
--- a/rust/helpers/platform.c
+++ b/rust/helpers/platform.c
@@ -2,16 +2,6 @@
 
 #include <linux/platform_device.h>
 
-void *rust_helper_platform_get_drvdata(const struct platform_device *pdev)
-{
-	return platform_get_drvdata(pdev);
-}
-
-void rust_helper_platform_set_drvdata(struct platform_device *pdev, void *data)
-{
-	platform_set_drvdata(pdev, data);
-}
-
 bool rust_helper_dev_is_platform(const struct device *dev)
 {
 	return dev_is_platform(dev);
diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index 5b21fa517e55..dc0c36d70963 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -6,11 +6,11 @@
 
 use crate::{
     bindings, container_of, device, driver,
-    error::{to_result, Result},
+    error::{from_result, to_result, Result},
     of,
     prelude::*,
     str::CStr,
-    types::{ForeignOwnable, Opaque},
+    types::Opaque,
     ThisModule,
 };
 
@@ -61,30 +61,28 @@ extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> kernel::ff
         // `struct platform_device`.
         //
         // 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>>() };
         let info = <Self as driver::Adapter>::id_info(pdev.as_ref());
-        match T::probe(pdev, info) {
-            Ok(data) => {
-                // Let the `struct platform_device` own a reference of the driver's private data.
-                // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
-                // `struct platform_device`.
-                unsafe { bindings::platform_set_drvdata(pdev.as_raw(), data.into_foreign() as _) };
-            }
-            Err(err) => return Error::to_errno(err),
-        }
 
-        0
+        from_result(|| {
+            let data = T::probe(pdev, info)?;
+
+            pdev.as_ref().set_drvdata(data);
+            Ok(0)
+        })
     }
 
     extern "C" fn remove_callback(pdev: *mut bindings::platform_device) {
-        // SAFETY: `pdev` is a valid pointer to a `struct platform_device`.
-        let ptr = unsafe { bindings::platform_get_drvdata(pdev) }.cast();
+        // SAFETY: The platform bus only ever calls the remove callback with a valid pointer to a
+        // `struct platform_device`.
+        //
+        // INVARIANT: `pdev` is valid for the duration of `probe_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>>>() };
     }
 }
 
-- 
2.49.0

[PATCH 4/8] rust: pci: use generic device drvdata accessors

[Danilo Krummrich]

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 <dakr@kernel.org>
---
 rust/helpers/pci.c | 10 ----------
 rust/kernel/pci.rs | 31 ++++++++++++++-----------------
 2 files changed, 14 insertions(+), 27 deletions(-)

diff --git a/rust/helpers/pci.c b/rust/helpers/pci.c
index cd0e6bf2cc4d..ef9cb38c81a6 100644
--- a/rust/helpers/pci.c
+++ b/rust/helpers/pci.c
@@ -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);
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 8435f8132e38..064e74a90904 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -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>>>() };
     }
 }
 
-- 
2.49.0

[PATCH 5/8] rust: auxiliary: use generic device drvdata accessors

[Danilo Krummrich]

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 <dakr@kernel.org>
---
 rust/helpers/auxiliary.c | 10 ----------
 rust/kernel/auxiliary.rs | 35 +++++++++++++++--------------------
 2 files changed, 15 insertions(+), 30 deletions(-)

diff --git a/rust/helpers/auxiliary.c b/rust/helpers/auxiliary.c
index 0db3860d774e..8b5e0fea4493 100644
--- a/rust/helpers/auxiliary.c
+++ b/rust/helpers/auxiliary.c
@@ -2,16 +2,6 @@
 
 #include <linux/auxiliary_bus.h>
 
-void rust_helper_auxiliary_set_drvdata(struct auxiliary_device *adev, void *data)
-{
-	auxiliary_set_drvdata(adev, data);
-}
-
-void *rust_helper_auxiliary_get_drvdata(struct auxiliary_device *adev)
-{
-	return auxiliary_get_drvdata(adev);
-}
-
 void rust_helper_auxiliary_device_uninit(struct auxiliary_device *adev)
 {
 	return auxiliary_device_uninit(adev);
diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs
index d2cfe1eeefb6..250d3178c334 100644
--- a/rust/kernel/auxiliary.rs
+++ b/rust/kernel/auxiliary.rs
@@ -8,10 +8,10 @@
     bindings, container_of, device,
     device_id::RawDeviceId,
     driver,
-    error::{to_result, Result},
+    error::{from_result, to_result, Result},
     prelude::*,
     str::CStr,
-    types::{ForeignOwnable, Opaque},
+    types::Opaque,
     ThisModule,
 };
 use core::{
@@ -61,37 +61,32 @@ extern "C" fn probe_callback(
         // `struct auxiliary_device`.
         //
         // INVARIANT: `adev` is valid for the duration of `probe_callback()`.
-        let adev = unsafe { &*adev.cast::<Device<device::Core>>() };
+        let adev = unsafe { &*adev.cast::<Device<device::Internal>>() };
 
         // SAFETY: `DeviceId` is a `#[repr(transparent)`] wrapper of `struct auxiliary_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(adev, info) {
-            Ok(data) => {
-                // Let the `struct auxiliary_device` own a reference of the driver's private data.
-                // SAFETY: By the type invariant `adev.as_raw` returns a valid pointer to a
-                // `struct auxiliary_device`.
-                unsafe {
-                    bindings::auxiliary_set_drvdata(adev.as_raw(), data.into_foreign().cast())
-                };
-            }
-            Err(err) => return Error::to_errno(err),
-        }
+        from_result(|| {
+            let data = T::probe(adev, info)?;
 
-        0
+            adev.as_ref().set_drvdata(data);
+            Ok(0)
+        })
     }
 
     extern "C" fn remove_callback(adev: *mut bindings::auxiliary_device) {
-        // SAFETY: The auxiliary bus only ever calls the remove callback with a valid pointer to a
+        // SAFETY: The auxiliary bus only ever calls the probe callback with a valid pointer to a
         // `struct auxiliary_device`.
-        let ptr = unsafe { bindings::auxiliary_get_drvdata(adev) };
+        //
+        // INVARIANT: `adev` is valid for the duration of `probe_callback()`.
+        let adev = unsafe { &*adev.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`.
-        drop(unsafe { KBox::<T>::from_foreign(ptr.cast()) });
+        // `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>>>() });
     }
 }
 
-- 
2.49.0

[PATCH 6/8] rust: platform: implement Driver::unbind()

[Danilo Krummrich]

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

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

diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index dc0c36d70963..9b3de89dc2f9 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -82,7 +82,9 @@ 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 _ = unsafe { pdev.as_ref().drvdata_obtain::<Pin<KBox<T>>>() };
+        let data = unsafe { pdev.as_ref().drvdata_obtain::<Pin<KBox<T>>>() };
+
+        T::unbind(pdev, data.as_ref());
     }
 }
 
@@ -164,6 +166,20 @@ pub trait Driver: Send {
     /// Implementers should attempt to initialize the device here.
     fn probe(dev: &Device<device::Core>, id_info: Option<&Self::IdInfo>)
         -> Result<Pin<KBox<Self>>>;
+
+    /// Platform driver unbind.
+    ///
+    /// Called when a [`Device`] is unbound from its bound [`Driver`]. Implementing this callback
+    /// is optional.
+    ///
+    /// This callback serves as a place for drivers to perform teardown operations that require a
+    /// `&Device<Core>` or `&Device<Bound>` reference. For instance, drivers may try to perform I/O
+    /// operations to gracefully tear down the device.
+    ///
+    /// Otherwise, release operations for driver resources should be performed in `Self::drop`.
+    fn unbind(dev: &Device<device::Core>, this: Pin<&Self>) {
+        let _ = (dev, this);
+    }
 }
 
 /// The platform device representation.
-- 
2.49.0

[PATCH 7/8] rust: pci: implement Driver::unbind()

[Danilo Krummrich]

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

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

diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 064e74a90904..6bdd3ab23f17 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -91,7 +91,9 @@ 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 _ = unsafe { pdev.as_ref().drvdata_obtain::<Pin<KBox<T>>>() };
+        let data = unsafe { pdev.as_ref().drvdata_obtain::<Pin<KBox<T>>>() };
+
+        T::unbind(pdev, data.as_ref());
     }
 }
 
@@ -238,6 +240,20 @@ pub trait Driver: Send {
     /// Called when a new platform device is added or discovered.
     /// Implementers should attempt to initialize the device here.
     fn probe(dev: &Device<device::Core>, id_info: &Self::IdInfo) -> Result<Pin<KBox<Self>>>;
+
+    /// Platform driver unbind.
+    ///
+    /// Called when a [`Device`] is unbound from its bound [`Driver`]. Implementing this callback
+    /// is optional.
+    ///
+    /// This callback serves as a place for drivers to perform teardown operations that require a
+    /// `&Device<Core>` or `&Device<Bound>` reference. For instance, drivers may try to perform I/O
+    /// operations to gracefully tear down the device.
+    ///
+    /// Otherwise, release operations for driver resources should be performed in `Self::drop`.
+    fn unbind(dev: &Device<device::Core>, this: Pin<&Self>) {
+        let _ = (dev, this);
+    }
 }
 
 /// The PCI device representation.
-- 
2.49.0

[PATCH 8/8] samples: rust: pci: reset pci-testdev in unbind()

[Danilo Krummrich]

Reset the pci-testdev when the driver is unbound from its device.

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

diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
index 15147e4401b2..062a242f8874 100644
--- a/samples/rust/rust_driver_pci.rs
+++ b/samples/rust/rust_driver_pci.rs
@@ -18,7 +18,7 @@ impl Regs {
 
 type Bar0 = pci::Bar<{ Regs::END }>;
 
-#[derive(Debug)]
+#[derive(Copy, Clone, Debug)]
 struct TestIndex(u8);
 
 impl TestIndex {
@@ -28,6 +28,7 @@ impl TestIndex {
 struct SampleDriver {
     pdev: ARef<pci::Device>,
     bar: Devres<Bar0>,
+    index: TestIndex,
 }
 
 kernel::pci_device_table!(
@@ -79,6 +80,7 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> Result<Pin<KBox<Self>
             Self {
                 pdev: pdev.into(),
                 bar,
+                index: *info,
             },
             GFP_KERNEL,
         )?;
@@ -92,6 +94,13 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> Result<Pin<KBox<Self>
 
         Ok(drvdata.into())
     }
+
+    fn unbind(pdev: &pci::Device<Core>, this: Pin<&Self>) {
+        if let Ok(bar) = this.bar.access(pdev.as_ref()) {
+            // Reset pci-testdev by writing a new test index.
+            bar.write8(this.index.0, Regs::TEST);
+        }
+    }
 }
 
 impl Drop for SampleDriver {
-- 
2.49.0

Re: [PATCH 0/8] Device: generic accessors for drvdata + Driver::unbind()

[Greg KH]

On Sat, Jun 21, 2025 at 09:43:26PM +0200, Danilo Krummrich wrote:
> 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.

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?

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

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.

thanks,

greg k-h

Re: [PATCH 1/8] rust: device: introduce device::Internal

[Greg KH]

On Sat, Jun 21, 2025 at 09:43:27PM +0200, Danilo Krummrich wrote:
> 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 <dakr@kernel.org>
> ---
>  rust/kernel/device.rs | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index 665f5ceadecc..e9094d8322d5 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -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 :)

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?

thanks,

greg k-h

Re: [PATCH 2/8] rust: device: add drvdata accessors

[Greg KH]

On Sat, Jun 21, 2025 at 09:43:28PM +0200, Danilo Krummrich wrote:
> 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 <dakr@kernel.org>
> ---
>  rust/helpers/device.c | 10 ++++++++++
>  rust/kernel/device.rs | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/helpers/device.c b/rust/helpers/device.c
> index b2135c6686b0..9bf252649c75 100644
> --- a/rust/helpers/device.c
> +++ b/rust/helpers/device.c
> @@ -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);
> +}
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index e9094d8322d5..146eba147d2f 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -6,7 +6,7 @@
>  
>  use crate::{
>      bindings,
> -    types::{ARef, Opaque},
> +    types::{ARef, ForeignOwnable, Opaque},
>  };
>  use core::{fmt, marker::PhantomData, ptr};
>  
> @@ -62,6 +62,47 @@ pub unsafe fn get_device(ptr: *mut bindings::device) -> ARef<Self> {
>      }
>  }
>  
> +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.


> +
> +    /// 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()) };
> +
> +        // 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?

thanks,

greg k-h

Re: [PATCH 4/8] rust: pci: use generic device drvdata accessors

[Greg KH]

On Sat, Jun 21, 2025 at 09:43:30PM +0200, Danilo Krummrich wrote:
> 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 <dakr@kernel.org>
> ---
>  rust/helpers/pci.c | 10 ----------
>  rust/kernel/pci.rs | 31 ++++++++++++++-----------------
>  2 files changed, 14 insertions(+), 27 deletions(-)
> 
> diff --git a/rust/helpers/pci.c b/rust/helpers/pci.c
> index cd0e6bf2cc4d..ef9cb38c81a6 100644
> --- a/rust/helpers/pci.c
> +++ b/rust/helpers/pci.c
> @@ -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);
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 8435f8132e38..064e74a90904 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -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>>>() };
>      }
>  }
>  
> -- 
> 2.49.0
> 
> 

Overall, I like this, same for the other bus types.  But again, can't it
be part of Core?

thanks,

greg k-h

Re: [PATCH 0/8] Device: generic accessors for drvdata + Driver::unbind()

[Danilo Krummrich]

On Tue, Jul 01, 2025 at 11:25:41AM +0200, Greg KH wrote:
> On Sat, Jun 21, 2025 at 09:43:26PM +0200, Danilo Krummrich wrote:
> > 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.
> 
> 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?

Not exactly, the thing in drop() is the driver's private data, which has the
exact lifetime of a driver being bound to a device, which makes drop() pretty
much identical to remove() in this aspect.

	// This is the private data of the driver.
	struct MyDriver {
	   bar: Devres<pci::Bar>,
	   ...
	}

	impl pci::Driver for MyDriver {
	   fn probe(
	      pdev: &pci::Device<Core>,
	      info: &Self::IdInfo
	   ) -> Result<Pin<KBox<Self>>> {
	      let bar = ...;
	      pdev.enable_device()?;

	      KBox::pin_init(Self { bar }, GFP_KERNEL)
	   }

	   fn unbind(&self, pdev: &Device<Core>) {
	      // Can only be called with a `&Device<Core>`.
	      pdev.disable_device();
	   }
	}

	impl Drop for MyDriver {
	   fn drop(&mut self) {
	      // We don't need to do anything here, the destructor of `self.bar`
	      // is called automatically, which is where the PCI BAR is unmapped
	      // and the resource region is released. In fact, this impl block
	      // is optional and can be omitted.
	   }
	}

The probe() method's return value is the driver's private data, which, due to
being a ForeignOwnable, is stored in dev->driver_data by the bus abstraction.

The lifetime goes until remove(), which is where the bus abstraction does not
borrow dev->driver_data, but instead re-creates the original driver data object,
which subsequently in the bus abstraction's remove() function goes out of scope
and hence is dropped.

From the bus abstraction side of things it conceptually looks like this:

	 extern "C" fn probe_callback(pdev, ...) {
	    let data = T::probe();

	    pdev.as_ref().set_drvdata(data);
	 }

	extern "C" fn remove_callback(pdev) {
	   let data = unsafe { pdev.as_ref().drvdata_obtain::<Pin<KBox<T>>>() }

	   T::unbind(pdev, data.as_ref());
	} // data.drop() is called here, since data goes out of scope.

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

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

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

For instance, some functions can only be implemented for a &Device<Core>, such
as pci_disable_device(), which hence we can't call from drop().

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

Why do you think so? To me it seems pretty clean to separate the "unbind()
teardown sequence" from the destructor of the driver's private data, even if
there wouldn't be a technical reason to do so.

Re: [PATCH 1/8] rust: device: introduce device::Internal

[Danilo Krummrich]

On Tue, Jul 01, 2025 at 11:26:47AM +0200, Greg KH wrote:
> On Sat, Jun 21, 2025 at 09:43:27PM +0200, Danilo Krummrich wrote:
> > 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 <dakr@kernel.org>
> > ---
> >  rust/kernel/device.rs | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> > index 665f5ceadecc..e9094d8322d5 100644
> > --- a/rust/kernel/device.rs
> > +++ b/rust/kernel/device.rs
> > @@ -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 :)
> 
> As this is ONLY for the bus code to touch, why not call it Bus_Internal?

BusInternal is better indeed!

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

Re: [PATCH 2/8] rust: device: add drvdata accessors

[Danilo Krummrich]

On Tue, Jul 01, 2025 at 11:27:54AM +0200, Greg KH wrote:
> On Sat, Jun 21, 2025 at 09:43:28PM +0200, Danilo Krummrich wrote:

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

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

> > +
> > +    /// 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()) };
> > +
> > +        // 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.

Re: [PATCH 1/8] rust: device: introduce device::Internal

[Danilo Krummrich]

On Tue, Jul 01, 2025 at 12:41:53PM +0200, Danilo Krummrich wrote:
> On Tue, Jul 01, 2025 at 11:26:47AM +0200, Greg KH wrote:
> > On Sat, Jun 21, 2025 at 09:43:27PM +0200, Danilo Krummrich wrote:
> > > 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 <dakr@kernel.org>
> > > ---
> > >  rust/kernel/device.rs | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > > 
> > > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> > > index 665f5ceadecc..e9094d8322d5 100644
> > > --- a/rust/kernel/device.rs
> > > +++ b/rust/kernel/device.rs
> > > @@ -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 :)
> > 
> > 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.

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

Re: [PATCH 2/8] rust: device: add drvdata accessors

[Danilo Krummrich]

On Tue, Jul 01, 2025 at 12:58:24PM +0200, Danilo Krummrich wrote:
> On Tue, Jul 01, 2025 at 11:27:54AM +0200, Greg KH wrote:
> > On Sat, Jun 21, 2025 at 09:43:28PM +0200, Danilo Krummrich wrote:
> 
> > > +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.
> 
> 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().
> 
> > > +
> > > +    /// 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()) };
> > > +
> > > +        // 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.

Also, I want to add that I already looked into implementing a safe drvdata()
accessor for &Device<Bound>.

&Device<Bound> would be fine, since the private driver data is guaranteed to be
valid as long as the device is bound to the driver.

(Note that we'd need to handle calling dev.drvdata() from probe() gracefully,
since probe() creates and returns the driver's private data and hence
dev->driver_data is only set subsequently in the bus abstraction's probe()
callback.)

The drvdata() accessor would also need to ensure that it returns the correct
type of the driver's private data, which the device itself does not know, which
requires additional complexity.

I have ideas for solving that, but I don't think we actually gonna need to
access the private data stored in the device from anywhere else than bus device
callbacks (I also don't think it is desirable), hence I'd really want to wait
and see a good reason for making this accessible.

Re: [PATCH 1/8] rust: device: introduce device::Internal

[Greg KH]

On Tue, Jul 01, 2025 at 02:32:48PM +0200, Danilo Krummrich wrote:
> On Tue, Jul 01, 2025 at 12:41:53PM +0200, Danilo Krummrich wrote:
> > On Tue, Jul 01, 2025 at 11:26:47AM +0200, Greg KH wrote:
> > > On Sat, Jun 21, 2025 at 09:43:27PM +0200, Danilo Krummrich wrote:
> > > > 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 <dakr@kernel.org>
> > > > ---
> > > >  rust/kernel/device.rs | 14 ++++++++++++++
> > > >  1 file changed, 14 insertions(+)
> > > > 
> > > > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> > > > index 665f5ceadecc..e9094d8322d5 100644
> > > > --- a/rust/kernel/device.rs
> > > > +++ b/rust/kernel/device.rs
> > > > @@ -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 :)
> > > 
> > > 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.

Thanks for the long explainations that helped out a lot.  As I said on
chat earlier, I agree with you now.  Can you respin this with
CoreInternal and we can queue it up?

Worst thing that happens is the api doesn't work out and we rework it
based on real users :)

thanks,

greg k-h

Re: [PATCH 2/8] rust: device: add drvdata accessors

[Benno Lossin]

On Sat Jun 21, 2025 at 9:43 PM CEST, Danilo Krummrich wrote:
> +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()`.

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.

---
Cheers,
Benno

> +        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 {

Re: [PATCH 2/8] rust: device: add drvdata accessors

[Danilo Krummrich]

On Sat, Jul 05, 2025 at 01:15:06PM +0200, Benno Lossin wrote:
> On Sat Jun 21, 2025 at 9:43 PM CEST, Danilo Krummrich wrote:
> > +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()`.
> 
> 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.

Great! What do you think about:

diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 146eba147d2f..b01cb8e8dab3 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -80,8 +80,11 @@ 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()`.
+        // SAFETY:
+        // - By the safety requirements of this function, `ptr` comes from a previous call to
+        //   `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()) }
     }

Re: [PATCH 2/8] rust: device: add drvdata accessors

[Benno Lossin]

On Sat Jul 5, 2025 at 5:06 PM CEST, Danilo Krummrich wrote:
> On Sat, Jul 05, 2025 at 01:15:06PM +0200, Benno Lossin wrote:
>> On Sat Jun 21, 2025 at 9:43 PM CEST, Danilo Krummrich wrote:
>> > +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()`.
>> 
>> 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.
>
> Great! What do you think about:
>
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index 146eba147d2f..b01cb8e8dab3 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -80,8 +80,11 @@ 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()`.
> +        // SAFETY:
> +        // - By the safety requirements of this function, `ptr` comes from a previous call to
> +        //   `into_foreign()`.
> +        // - `dev_get_drvdata()` guarantees to return the same pointer given to `dev_set_drvdata()`
> +        //   in `into_foreign()`.

Looks good, though I haven't done a full review, but you can have my:

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

---
Cheers,
Benno

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

Re: [PATCH 0/8] Device: generic accessors for drvdata + Driver::unbind()

[Alexandre Courbot]

On Tue Jul 1, 2025 at 7:40 PM JST, Danilo Krummrich wrote:
>> > 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).
>> 
>> 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.
>
>> 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().

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?

Re: [PATCH 2/8] rust: device: add drvdata accessors

[Alexandre Courbot]

On Sun Jun 22, 2025 at 4:43 AM JST, Danilo Krummrich wrote:
<snip>
> +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
> +    ///   [`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()) }
> +    }
> +}

This is a comment triggered by an intuition, so please ignore it if it
doesn't make any sense (it probably doesn't :)).

I have a hunch that we could make more of the methods above safe by
either introducing a typestate to `Internal`, or (which comes down to
the same) using two separate device contexts, one used until
`set_drvdata` is called, and one after, the latter being able to provide
a safe implementation of `drvdata_borrow` (since we know that
`set_drvdata` must have been called).

Since buses must do an unsafe cast to `Device<Internal>` anyway, why not
encode the driver's data type and whether the driver data has been set
or not in that cast as well. E.g, instead of having:

    let pdev = unsafe { &*pdev.cast::<Device<Internal>>() };
    ...
    let foo = unsafe { pdev.as_ref().drvdata_borrow::<Pin<KBox<T>>>() };

You would do:

    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.
^_^;

And if we can only move `set_drvdata` somewhere else (not sure where
though), then we could assume that `Internal` always has its driver data
set and deal with a single context.

I don't think the design of `Device` allows us to work with anything
else then references to it, so it is unlikely that we can make
`set_drvdata` and `drvdata_obtain` morph its type, which is unfortunate
as it would have allowed us to make these methods safe as well.

Re: [PATCH 0/8] Device: generic accessors for drvdata + Driver::unbind()

[Danilo Krummrich]

On Mon, Jul 07, 2025 at 04:18:09PM +0900, Alexandre Courbot wrote:
> On Tue Jul 1, 2025 at 7:40 PM JST, Danilo Krummrich wrote:
> >> > 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).
> >> 
> >> 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.
> >
> >> 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().
> 
> 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?

No, we could do that. And I thought about this as well, but I really want to
bind the lifetime of the driver's private data stored in a device to the
lifetime of this driver being bound to the device.

I don't think there is a valid use-case to allow this data as a whole to
out-live driver unbind arbitrarily.

Re: [PATCH 2/8] rust: device: add drvdata accessors

[Danilo Krummrich]

On Mon, Jul 07, 2025 at 04:46:09PM +0900, Alexandre Courbot wrote:
>     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 think this doesn't remove the safety requirement. drvdata_borrow() or
drvdata_obtain() would still require CoreInternal<Pin<KBox<T>>> to have the
correct type generic.

Maybe we could have some invariant on CoreInternal that the generic type is
*always* the bound driver's private data type. But then we have an
`// INVARIANT` comment on the `Device<CoreInternal<...>>` cast, which would need
the same justification as the current safety requirement.

So, I don't think this safety requirement goes away. You can only move it
around.

> 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.
> ^_^;

It will be used from other bus callbacks, such as shutdown(). (There are bus
abstractions on the list (e.g. I2C) that will start using it in the next
cycle.)

Re: [PATCH 0/8] Device: generic accessors for drvdata + Driver::unbind()

[Danilo Krummrich]

On Sat, Jun 21, 2025 at 09:43:26PM +0200, Danilo Krummrich wrote:
>   rust: device: introduce device::Internal

  [ Rename device::Internal to device::CoreInternal. - Danilo ]

>   rust: device: add drvdata accessors

  [ Improve safety comment as proposed by Benno. - Danilo ]

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

Applied to driver-core-testing, thanks!