[PATCH 0/6] Address race condition with Device::drvdata()

[PATCH 0/6] Address race condition with Device::drvdata()

[Danilo Krummrich]

Currently, the driver's device private data is allocated and initialized
from driver core code called from bus abstractions after the driver's
probe() callback returned the corresponding initializer.

Similarly, the driver's device private data is dropped within the
remove() callback of bus abstractions after calling the remove()
callback of the corresponding driver.

However, commit 6f61a2637abe ("rust: device: introduce
Device::drvdata()") introduced an accessor for the driver's device
private data for a Device<Bound>, i.e. a device that is currently bound
to a driver.

Obviously, this is in conflict with dropping the driver's device private
data in remove(), since a device can not be considered to be fully
unbound after remove() has finished:

We also have to consider registrations guarded by devres - such as IRQ
or class device registrations - which are torn down after remove() in
devres_release_all().

Thus, it can happen that, for instance, a class device or IRQ callback
still calls Device::drvdata(), which then runs concurrently to remove()
(which sets dev->driver_data to NULL and drops the driver's device
private data), before devres_release_all() started to tear down the
corresponding registration. This is because devres guarded registrations
can, as expected, access the corresponding Device<Bound> that defines
their scope.

In C it simply is the driver's responsibility to ensure that its device
private data is freed after e.g. an IRQ registration is unregistered.

Typically, C drivers achieve this by allocating their device private data
with e.g. devm_kzalloc() before doing anything else, i.e. before e.g.
registering an IRQ with devm_request_threaded_irq(), relying on the
reverse order cleanup of devres [1].

Technically, we could do something similar in Rust. However, the
resulting code would be pretty messy:

In Rust we have to differentiate between allocated but uninitialized
memory and initialized memory in the type system. Thus, we would need to
somehow keep track of whether the driver's device private data object
has been initialized (i.e. probe() was successful and returned a valid
initializer for this memory) and conditionally call the destructor of
the corresponding object when it is freed.

This is because we'd need to allocate and register the memory of the
driver's device private data *before* it is initialized by the
initializer returned by the driver's probe() callback, because the
driver could already register devres guarded registrations within
probe() outside of the driver's device private data initializer.

Luckily there is a much simpler solution: Instead of dropping the
driver's device private data at the end of remove(), we just drop it
after the device has been fully unbound, i.e. after all devres callbacks
have been processed.

For this, we introduce a new post_unbind() callback private to the
driver-core, i.e. the callback is neither exposed to drivers, nor to bus
abstractions.

This way, the driver-core code can simply continue to conditionally
allocate the memory for the driver's device private data when the
driver's initializer is returned from probe() - no change needed - and
drop it when the driver-core code receives the post_unbind() callback.

--

Dependency wise we need a common Driver trait that describes the layout of a
specific driver structure, such as struct pci_driver or struct platform_driver.
Additional to this specific driver type (which was previously the associated
type RegType of the RegistrationOps) it provides the offset to the embedded
struct device_driver and the type of the driver's device private data.

This patch series contains two additional dependencies:

  (1) A fix for i2c::Driver::shutdown() to not free the driver's device
      private data at all, which otherwise causes the exact same bug, and
      is not necessary in the first place anyways.

  (2) Add the auxiliary::Driver::unbind() callback. Strictly speaking,
      this is not a dependency, but without this patch the main fix of this
      series leaves the remove() callback of the auxiliary bus
      abstraction with either dead code or quite some code removed;
      code that we would otherwise add back immediately afterwards.

--

[1] In fact, the cleanup ordering of devres is a separate challenge in
    Rust, since it is technically unsound to rely on the driver to pick
    the correct order. I am already working on a solution for this;
    luckily this also has some synergies with optimizing the required
    synchronize_rcu() calls required by the Rust Devres container
    structure down to exactly one per driver unbind.

Link: https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=driver/post_unbind

Danilo Krummrich (6):
  rust: i2c: do not drop device private data on shutdown()
  rust: auxiliary: add Driver::unbind() callback
  rust: driver: introduce a common Driver trait
  rust: driver: add DEVICE_DRIVER_OFFSET to the Driver trait
  rust: driver: add DriverData type to the generic Driver trait
  rust: driver: drop device private data post unbind

 drivers/base/dd.c             |  4 ++
 include/linux/device/driver.h | 11 +++++
 rust/kernel/auxiliary.rs      | 41 +++++++++++++----
 rust/kernel/device.rs         | 20 ++++----
 rust/kernel/driver.rs         | 86 ++++++++++++++++++++++++++++-------
 rust/kernel/i2c.rs            | 31 ++++++++-----
 rust/kernel/pci.rs            | 27 +++++++----
 rust/kernel/platform.rs       | 27 +++++++----
 rust/kernel/usb.rs            | 27 +++++++----
 9 files changed, 203 insertions(+), 71 deletions(-)


base-commit: 8510ef5e3cfbd7d59a16845f85cd0194a8689761
-- 
2.52.0

[PATCH 1/6] rust: i2c: do not drop device private data on shutdown()

[Danilo Krummrich]

We must not drop the device private data on shutdown(); none of the
registrations attached to devres that might access the device private
data are released before shutdown() is called.

Hence, freeing the device private data on shutdown() can cause UAF bugs.

Fixes: 57c5bd9aee94 ("rust: i2c: add basic I2C device and driver abstractions")
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/i2c.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
index 491e6cc25cf4..35b678b78d91 100644
--- a/rust/kernel/i2c.rs
+++ b/rust/kernel/i2c.rs
@@ -181,9 +181,9 @@ extern "C" fn shutdown_callback(idev: *mut bindings::i2c_client) {
         // SAFETY: `shutdown_callback` is only ever called after a successful call to
         // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
         // and stored a `Pin<KBox<T>>`.
-        let data = unsafe { idev.as_ref().drvdata_obtain::<T>() };
+        let data = unsafe { idev.as_ref().drvdata_borrow::<T>() };
 
-        T::shutdown(idev, data.as_ref());
+        T::shutdown(idev, data);
     }
 
     /// The [`i2c::IdTable`] of the corresponding driver.
-- 
2.52.0

[PATCH 2/6] rust: auxiliary: add Driver::unbind() callback

[Danilo Krummrich]

Add missing unbind() callback to auxiliary::Driver, since it will be
needed by drivers eventually (e.g. the Nova DRM driver).

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 Strictly speaking, this is not a dependency, but without this patch the main
 fix of this series leaves the remove() callback of the auxiliary bus
 abstraction with either dead code or quite some code removed; code that we
 would otherwise add back immediately afterwards.
---
 rust/kernel/auxiliary.rs | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs
index 56f3c180e8f6..6931f8a4267f 100644
--- a/rust/kernel/auxiliary.rs
+++ b/rust/kernel/auxiliary.rs
@@ -87,7 +87,9 @@ extern "C" fn remove_callback(adev: *mut bindings::auxiliary_device) {
         // SAFETY: `remove_callback` is only ever called after a successful call to
         // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
         // and stored a `Pin<KBox<T>>`.
-        drop(unsafe { adev.as_ref().drvdata_obtain::<T>() });
+        let data = unsafe { adev.as_ref().drvdata_obtain::<T>() };
+
+        T::unbind(adev, data.as_ref());
     }
 }
 
@@ -187,6 +189,20 @@ pub trait Driver {
     ///
     /// Called when an auxiliary device is matches a corresponding driver.
     fn probe(dev: &Device<device::Core>, id_info: &Self::IdInfo) -> impl PinInit<Self, Error>;
+
+    /// Auxiliary 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 auxiliary device representation.
-- 
2.52.0

[PATCH 3/6] rust: driver: introduce a common Driver trait

[Danilo Krummrich]

The Driver trait describes the layout of a specific driver structure,
such as `struct pci_driver` or `struct platform_driver`.

In a first step, this replaces the associated type RegType of the
RegistrationOps with the Driver::DriverType associated type.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/auxiliary.rs | 18 +++++++++++-------
 rust/kernel/driver.rs    | 40 +++++++++++++++++++++++++---------------
 rust/kernel/i2c.rs       | 18 +++++++++++-------
 rust/kernel/pci.rs       | 18 +++++++++++-------
 rust/kernel/platform.rs  | 18 +++++++++++-------
 rust/kernel/usb.rs       | 18 +++++++++++-------
 6 files changed, 80 insertions(+), 50 deletions(-)

diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs
index 6931f8a4267f..4636b6f41195 100644
--- a/rust/kernel/auxiliary.rs
+++ b/rust/kernel/auxiliary.rs
@@ -23,13 +23,17 @@
 /// An adapter for the registration of auxiliary drivers.
 pub struct Adapter<T: Driver>(T);
 
-// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if
+// SAFETY:
+// - `bindings::auxiliary_driver` is a C type declared as `repr(C)`.
+unsafe impl<T: Driver + 'static> driver::Driver for Adapter<T> {
+    type DriverType = bindings::auxiliary_driver;
+}
+
+// SAFETY: A call to `unregister` for a given instance of `DriverType` is guaranteed to be valid if
 // a preceding call to `register` has been successful.
 unsafe impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
-    type RegType = bindings::auxiliary_driver;
-
     unsafe fn register(
-        adrv: &Opaque<Self::RegType>,
+        adrv: &Opaque<Self::DriverType>,
         name: &'static CStr,
         module: &'static ThisModule,
     ) -> Result {
@@ -41,14 +45,14 @@ unsafe fn register(
             (*adrv.get()).id_table = T::ID_TABLE.as_ptr();
         }
 
-        // SAFETY: `adrv` is guaranteed to be a valid `RegType`.
+        // SAFETY: `adrv` is guaranteed to be a valid `DriverType`.
         to_result(unsafe {
             bindings::__auxiliary_driver_register(adrv.get(), module.0, name.as_char_ptr())
         })
     }
 
-    unsafe fn unregister(adrv: &Opaque<Self::RegType>) {
-        // SAFETY: `adrv` is guaranteed to be a valid `RegType`.
+    unsafe fn unregister(adrv: &Opaque<Self::DriverType>) {
+        // SAFETY: `adrv` is guaranteed to be a valid `DriverType`.
         unsafe { bindings::auxiliary_driver_unregister(adrv.get()) }
     }
 }
diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs
index 649d06468f41..cd1d36c313e1 100644
--- a/rust/kernel/driver.rs
+++ b/rust/kernel/driver.rs
@@ -99,23 +99,33 @@
 use core::pin::Pin;
 use pin_init::{pin_data, pinned_drop, PinInit};
 
+/// Trait describing the layout of a specific device driver.
+///
+/// This trait describes the layout of a specific driver structure, such as `struct pci_driver` or
+/// `struct platform_driver`.
+///
+/// # Safety
+///
+/// Implementors must guarantee that:
+/// - `DriverType` is `repr(C)`.
+pub unsafe trait Driver {
+    /// The specific driver type embedding a `struct device_driver`.
+    type DriverType: Default;
+}
+
 /// The [`RegistrationOps`] trait serves as generic interface for subsystems (e.g., PCI, Platform,
 /// Amba, etc.) to provide the corresponding subsystem specific implementation to register /
-/// unregister a driver of the particular type (`RegType`).
+/// unregister a driver of the particular type (`DriverType`).
 ///
-/// For instance, the PCI subsystem would set `RegType` to `bindings::pci_driver` and call
+/// For instance, the PCI subsystem would set `DriverType` to `bindings::pci_driver` and call
 /// `bindings::__pci_register_driver` from `RegistrationOps::register` and
 /// `bindings::pci_unregister_driver` from `RegistrationOps::unregister`.
 ///
 /// # Safety
 ///
-/// A call to [`RegistrationOps::unregister`] for a given instance of `RegType` is only valid if a
-/// preceding call to [`RegistrationOps::register`] has been successful.
-pub unsafe trait RegistrationOps {
-    /// The type that holds information about the registration. This is typically a struct defined
-    /// by the C portion of the kernel.
-    type RegType: Default;
-
+/// A call to [`RegistrationOps::unregister`] for a given instance of `DriverType` is only valid if
+/// a preceding call to [`RegistrationOps::register`] has been successful.
+pub unsafe trait RegistrationOps: Driver {
     /// Registers a driver.
     ///
     /// # Safety
@@ -123,7 +133,7 @@ pub unsafe trait RegistrationOps {
     /// On success, `reg` must remain pinned and valid until the matching call to
     /// [`RegistrationOps::unregister`].
     unsafe fn register(
-        reg: &Opaque<Self::RegType>,
+        reg: &Opaque<Self::DriverType>,
         name: &'static CStr,
         module: &'static ThisModule,
     ) -> Result;
@@ -134,7 +144,7 @@ unsafe fn register(
     ///
     /// Must only be called after a preceding successful call to [`RegistrationOps::register`] for
     /// the same `reg`.
-    unsafe fn unregister(reg: &Opaque<Self::RegType>);
+    unsafe fn unregister(reg: &Opaque<Self::DriverType>);
 }
 
 /// A [`Registration`] is a generic type that represents the registration of some driver type (e.g.
@@ -146,7 +156,7 @@ unsafe fn register(
 #[pin_data(PinnedDrop)]
 pub struct Registration<T: RegistrationOps> {
     #[pin]
-    reg: Opaque<T::RegType>,
+    reg: Opaque<T::DriverType>,
 }
 
 // SAFETY: `Registration` has no fields or methods accessible via `&Registration`, so it is safe to
@@ -161,13 +171,13 @@ impl<T: RegistrationOps> Registration<T> {
     /// Creates a new instance of the registration object.
     pub fn new(name: &'static CStr, module: &'static ThisModule) -> impl PinInit<Self, Error> {
         try_pin_init!(Self {
-            reg <- Opaque::try_ffi_init(|ptr: *mut T::RegType| {
+            reg <- Opaque::try_ffi_init(|ptr: *mut T::DriverType| {
                 // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write.
-                unsafe { ptr.write(T::RegType::default()) };
+                unsafe { ptr.write(T::DriverType::default()) };
 
                 // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write, and it has
                 // just been initialised above, so it's also valid for read.
-                let drv = unsafe { &*(ptr as *const Opaque<T::RegType>) };
+                let drv = unsafe { &*(ptr as *const Opaque<T::DriverType>) };
 
                 // SAFETY: `drv` is guaranteed to be pinned until `T::unregister`.
                 unsafe { T::register(drv, name, module) }
diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
index 35b678b78d91..de35961c6903 100644
--- a/rust/kernel/i2c.rs
+++ b/rust/kernel/i2c.rs
@@ -92,13 +92,17 @@ macro_rules! i2c_device_table {
 /// An adapter for the registration of I2C drivers.
 pub struct Adapter<T: Driver>(T);
 
-// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if
+// SAFETY:
+// - `bindings::i2c_driver` is a C type declared as `repr(C)`.
+unsafe impl<T: Driver + 'static> driver::Driver for Adapter<T> {
+    type DriverType = bindings::i2c_driver;
+}
+
+// SAFETY: A call to `unregister` for a given instance of `DriverType` is guaranteed to be valid if
 // a preceding call to `register` has been successful.
 unsafe impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
-    type RegType = bindings::i2c_driver;
-
     unsafe fn register(
-        idrv: &Opaque<Self::RegType>,
+        idrv: &Opaque<Self::DriverType>,
         name: &'static CStr,
         module: &'static ThisModule,
     ) -> Result {
@@ -133,12 +137,12 @@ unsafe fn register(
             (*idrv.get()).driver.acpi_match_table = acpi_table;
         }
 
-        // SAFETY: `idrv` is guaranteed to be a valid `RegType`.
+        // SAFETY: `idrv` is guaranteed to be a valid `DriverType`.
         to_result(unsafe { bindings::i2c_register_driver(module.0, idrv.get()) })
     }
 
-    unsafe fn unregister(idrv: &Opaque<Self::RegType>) {
-        // SAFETY: `idrv` is guaranteed to be a valid `RegType`.
+    unsafe fn unregister(idrv: &Opaque<Self::DriverType>) {
+        // SAFETY: `idrv` is guaranteed to be a valid `DriverType`.
         unsafe { bindings::i2c_del_driver(idrv.get()) }
     }
 }
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 82e128431f08..f58ce35d9c60 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -50,13 +50,17 @@
 /// An adapter for the registration of PCI drivers.
 pub struct Adapter<T: Driver>(T);
 
-// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if
+// SAFETY:
+// - `bindings::pci_driver` is a C type declared as `repr(C)`.
+unsafe impl<T: Driver + 'static> driver::Driver for Adapter<T> {
+    type DriverType = bindings::pci_driver;
+}
+
+// SAFETY: A call to `unregister` for a given instance of `DriverType` is guaranteed to be valid if
 // a preceding call to `register` has been successful.
 unsafe impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
-    type RegType = bindings::pci_driver;
-
     unsafe fn register(
-        pdrv: &Opaque<Self::RegType>,
+        pdrv: &Opaque<Self::DriverType>,
         name: &'static CStr,
         module: &'static ThisModule,
     ) -> Result {
@@ -68,14 +72,14 @@ unsafe fn register(
             (*pdrv.get()).id_table = T::ID_TABLE.as_ptr();
         }
 
-        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
+        // SAFETY: `pdrv` is guaranteed to be a valid `DriverType`.
         to_result(unsafe {
             bindings::__pci_register_driver(pdrv.get(), module.0, name.as_char_ptr())
         })
     }
 
-    unsafe fn unregister(pdrv: &Opaque<Self::RegType>) {
-        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
+    unsafe fn unregister(pdrv: &Opaque<Self::DriverType>) {
+        // SAFETY: `pdrv` is guaranteed to be a valid `DriverType`.
         unsafe { bindings::pci_unregister_driver(pdrv.get()) }
     }
 }
diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index ed889f079cab..e48d055fdc8a 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -26,13 +26,17 @@
 /// An adapter for the registration of platform drivers.
 pub struct Adapter<T: Driver>(T);
 
-// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if
+// SAFETY:
+// - `bindings::platform_driver` is a C type declared as `repr(C)`.
+unsafe impl<T: Driver + 'static> driver::Driver for Adapter<T> {
+    type DriverType = bindings::platform_driver;
+}
+
+// SAFETY: A call to `unregister` for a given instance of `DriverType` is guaranteed to be valid if
 // a preceding call to `register` has been successful.
 unsafe impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
-    type RegType = bindings::platform_driver;
-
     unsafe fn register(
-        pdrv: &Opaque<Self::RegType>,
+        pdrv: &Opaque<Self::DriverType>,
         name: &'static CStr,
         module: &'static ThisModule,
     ) -> Result {
@@ -55,12 +59,12 @@ unsafe fn register(
             (*pdrv.get()).driver.acpi_match_table = acpi_table;
         }
 
-        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
+        // SAFETY: `pdrv` is guaranteed to be a valid `DriverType`.
         to_result(unsafe { bindings::__platform_driver_register(pdrv.get(), module.0) })
     }
 
-    unsafe fn unregister(pdrv: &Opaque<Self::RegType>) {
-        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
+    unsafe fn unregister(pdrv: &Opaque<Self::DriverType>) {
+        // SAFETY: `pdrv` is guaranteed to be a valid `DriverType`.
         unsafe { bindings::platform_driver_unregister(pdrv.get()) };
     }
 }
diff --git a/rust/kernel/usb.rs b/rust/kernel/usb.rs
index d10b65e9fb6a..32f4b2d55dfb 100644
--- a/rust/kernel/usb.rs
+++ b/rust/kernel/usb.rs
@@ -27,13 +27,17 @@
 /// An adapter for the registration of USB drivers.
 pub struct Adapter<T: Driver>(T);
 
-// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if
+// SAFETY:
+// - `bindings::usb_driver` is a C type declared as `repr(C)`.
+unsafe impl<T: Driver + 'static> driver::Driver for Adapter<T> {
+    type DriverType = bindings::usb_driver;
+}
+
+// SAFETY: A call to `unregister` for a given instance of `DriverType` is guaranteed to be valid if
 // a preceding call to `register` has been successful.
 unsafe impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
-    type RegType = bindings::usb_driver;
-
     unsafe fn register(
-        udrv: &Opaque<Self::RegType>,
+        udrv: &Opaque<Self::DriverType>,
         name: &'static CStr,
         module: &'static ThisModule,
     ) -> Result {
@@ -45,14 +49,14 @@ unsafe fn register(
             (*udrv.get()).id_table = T::ID_TABLE.as_ptr();
         }
 
-        // SAFETY: `udrv` is guaranteed to be a valid `RegType`.
+        // SAFETY: `udrv` is guaranteed to be a valid `DriverType`.
         to_result(unsafe {
             bindings::usb_register_driver(udrv.get(), module.0, name.as_char_ptr())
         })
     }
 
-    unsafe fn unregister(udrv: &Opaque<Self::RegType>) {
-        // SAFETY: `udrv` is guaranteed to be a valid `RegType`.
+    unsafe fn unregister(udrv: &Opaque<Self::DriverType>) {
+        // SAFETY: `udrv` is guaranteed to be a valid `DriverType`.
         unsafe { bindings::usb_deregister(udrv.get()) };
     }
 }
-- 
2.52.0

[PATCH 4/6] rust: driver: add DEVICE_DRIVER_OFFSET to the Driver trait

[Danilo Krummrich]

Add an associated const DEVICE_DRIVER_OFFSET to the Driver trait
indicating the offset of the embedded struct device_driver within
Self::DriverType, i.e. the specific driver structs, such as
struct pci_driver or struct platform_driver.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/auxiliary.rs | 3 +++
 rust/kernel/driver.rs    | 8 +++++++-
 rust/kernel/i2c.rs       | 3 +++
 rust/kernel/pci.rs       | 3 +++
 rust/kernel/platform.rs  | 3 +++
 rust/kernel/usb.rs       | 3 +++
 6 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs
index 4636b6f41195..e712d1b89dc3 100644
--- a/rust/kernel/auxiliary.rs
+++ b/rust/kernel/auxiliary.rs
@@ -25,8 +25,11 @@
 
 // SAFETY:
 // - `bindings::auxiliary_driver` is a C type declared as `repr(C)`.
+// - `struct auxiliary_driver` embeds a `struct device_driver`.
+// - `DEVICE_DRIVER_OFFSET` is the correct byte offset to the embedded `struct device_driver`.
 unsafe impl<T: Driver + 'static> driver::Driver for Adapter<T> {
     type DriverType = bindings::auxiliary_driver;
+    const DEVICE_DRIVER_OFFSET: usize = core::mem::offset_of!(Self::DriverType, driver);
 }
 
 // SAFETY: A call to `unregister` for a given instance of `DriverType` is guaranteed to be valid if
diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs
index cd1d36c313e1..4b0c53b7d22a 100644
--- a/rust/kernel/driver.rs
+++ b/rust/kernel/driver.rs
@@ -107,10 +107,16 @@
 /// # Safety
 ///
 /// Implementors must guarantee that:
-/// - `DriverType` is `repr(C)`.
+/// - `DriverType` is `repr(C)`,
+/// - `DriverType` embeds a valid `struct device_driver` at byte offset `DEVICE_DRIVER_OFFSET`.
 pub unsafe trait Driver {
     /// The specific driver type embedding a `struct device_driver`.
     type DriverType: Default;
+
+    /// Byte offset of the embedded `struct device_driver` within `DriverType`.
+    ///
+    /// This must correspond exactly to the location of the embedded `struct device_driver` field.
+    const DEVICE_DRIVER_OFFSET: usize;
 }
 
 /// The [`RegistrationOps`] trait serves as generic interface for subsystems (e.g., PCI, Platform,
diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
index de35961c6903..56f1ed8163a0 100644
--- a/rust/kernel/i2c.rs
+++ b/rust/kernel/i2c.rs
@@ -94,8 +94,11 @@ macro_rules! i2c_device_table {
 
 // SAFETY:
 // - `bindings::i2c_driver` is a C type declared as `repr(C)`.
+// - `struct i2c_driver` embeds a `struct device_driver`.
+// - `DEVICE_DRIVER_OFFSET` is the correct byte offset to the embedded `struct device_driver`.
 unsafe impl<T: Driver + 'static> driver::Driver for Adapter<T> {
     type DriverType = bindings::i2c_driver;
+    const DEVICE_DRIVER_OFFSET: usize = core::mem::offset_of!(Self::DriverType, driver);
 }
 
 // SAFETY: A call to `unregister` for a given instance of `DriverType` is guaranteed to be valid if
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index f58ce35d9c60..68466150ef20 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -52,8 +52,11 @@
 
 // SAFETY:
 // - `bindings::pci_driver` is a C type declared as `repr(C)`.
+// - `struct pci_driver` embeds a `struct device_driver`.
+// - `DEVICE_DRIVER_OFFSET` is the correct byte offset to the embedded `struct device_driver`.
 unsafe impl<T: Driver + 'static> driver::Driver for Adapter<T> {
     type DriverType = bindings::pci_driver;
+    const DEVICE_DRIVER_OFFSET: usize = core::mem::offset_of!(Self::DriverType, driver);
 }
 
 // SAFETY: A call to `unregister` for a given instance of `DriverType` is guaranteed to be valid if
diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index e48d055fdc8a..56d9e968634e 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -28,8 +28,11 @@
 
 // SAFETY:
 // - `bindings::platform_driver` is a C type declared as `repr(C)`.
+// - `struct platform_driver` embeds a `struct device_driver`.
+// - `DEVICE_DRIVER_OFFSET` is the correct byte offset to the embedded `struct device_driver`.
 unsafe impl<T: Driver + 'static> driver::Driver for Adapter<T> {
     type DriverType = bindings::platform_driver;
+    const DEVICE_DRIVER_OFFSET: usize = core::mem::offset_of!(Self::DriverType, driver);
 }
 
 // SAFETY: A call to `unregister` for a given instance of `DriverType` is guaranteed to be valid if
diff --git a/rust/kernel/usb.rs b/rust/kernel/usb.rs
index 32f4b2d55dfb..a9a9d2298d87 100644
--- a/rust/kernel/usb.rs
+++ b/rust/kernel/usb.rs
@@ -29,8 +29,11 @@
 
 // SAFETY:
 // - `bindings::usb_driver` is a C type declared as `repr(C)`.
+// - `struct usb_driver` embeds a `struct device_driver`.
+// - `DEVICE_DRIVER_OFFSET` is the correct byte offset to the embedded `struct device_driver`.
 unsafe impl<T: Driver + 'static> driver::Driver for Adapter<T> {
     type DriverType = bindings::usb_driver;
+    const DEVICE_DRIVER_OFFSET: usize = core::mem::offset_of!(Self::DriverType, driver);
 }
 
 // SAFETY: A call to `unregister` for a given instance of `DriverType` is guaranteed to be valid if
-- 
2.52.0

[PATCH 5/6] rust: driver: add DriverData type to the generic Driver trait

[Danilo Krummrich]

Add an associated type DriverData to the Driver trait indicating the
type of the driver's device private data.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/auxiliary.rs | 2 ++
 rust/kernel/driver.rs    | 4 ++++
 rust/kernel/i2c.rs       | 2 ++
 rust/kernel/pci.rs       | 2 ++
 rust/kernel/platform.rs  | 2 ++
 rust/kernel/usb.rs       | 2 ++
 6 files changed, 14 insertions(+)

diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs
index e712d1b89dc3..cb26238e95b0 100644
--- a/rust/kernel/auxiliary.rs
+++ b/rust/kernel/auxiliary.rs
@@ -25,10 +25,12 @@
 
 // SAFETY:
 // - `bindings::auxiliary_driver` is a C type declared as `repr(C)`.
+// - `T` is the type of the driver's device private data.
 // - `struct auxiliary_driver` embeds a `struct device_driver`.
 // - `DEVICE_DRIVER_OFFSET` is the correct byte offset to the embedded `struct device_driver`.
 unsafe impl<T: Driver + 'static> driver::Driver for Adapter<T> {
     type DriverType = bindings::auxiliary_driver;
+    type DriverData = T;
     const DEVICE_DRIVER_OFFSET: usize = core::mem::offset_of!(Self::DriverType, driver);
 }
 
diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs
index 4b0c53b7d22a..77c1f7434897 100644
--- a/rust/kernel/driver.rs
+++ b/rust/kernel/driver.rs
@@ -108,11 +108,15 @@
 ///
 /// Implementors must guarantee that:
 /// - `DriverType` is `repr(C)`,
+/// - `DriverData` is the type of the driver's device private data.
 /// - `DriverType` embeds a valid `struct device_driver` at byte offset `DEVICE_DRIVER_OFFSET`.
 pub unsafe trait Driver {
     /// The specific driver type embedding a `struct device_driver`.
     type DriverType: Default;
 
+    /// The type of the driver's device private data.
+    type DriverData;
+
     /// Byte offset of the embedded `struct device_driver` within `DriverType`.
     ///
     /// This must correspond exactly to the location of the embedded `struct device_driver` field.
diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
index 56f1ed8163a0..6a3923a8b8a7 100644
--- a/rust/kernel/i2c.rs
+++ b/rust/kernel/i2c.rs
@@ -94,10 +94,12 @@ macro_rules! i2c_device_table {
 
 // SAFETY:
 // - `bindings::i2c_driver` is a C type declared as `repr(C)`.
+// - `T` is the type of the driver's device private data.
 // - `struct i2c_driver` embeds a `struct device_driver`.
 // - `DEVICE_DRIVER_OFFSET` is the correct byte offset to the embedded `struct device_driver`.
 unsafe impl<T: Driver + 'static> driver::Driver for Adapter<T> {
     type DriverType = bindings::i2c_driver;
+    type DriverData = T;
     const DEVICE_DRIVER_OFFSET: usize = core::mem::offset_of!(Self::DriverType, driver);
 }
 
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 68466150ef20..fe63b53d55d6 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -52,10 +52,12 @@
 
 // SAFETY:
 // - `bindings::pci_driver` is a C type declared as `repr(C)`.
+// - `T` is the type of the driver's device private data.
 // - `struct pci_driver` embeds a `struct device_driver`.
 // - `DEVICE_DRIVER_OFFSET` is the correct byte offset to the embedded `struct device_driver`.
 unsafe impl<T: Driver + 'static> driver::Driver for Adapter<T> {
     type DriverType = bindings::pci_driver;
+    type DriverData = T;
     const DEVICE_DRIVER_OFFSET: usize = core::mem::offset_of!(Self::DriverType, driver);
 }
 
diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index 56d9e968634e..af94fb58aafb 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -28,10 +28,12 @@
 
 // SAFETY:
 // - `bindings::platform_driver` is a C type declared as `repr(C)`.
+// - `T` is the type of the driver's device private data.
 // - `struct platform_driver` embeds a `struct device_driver`.
 // - `DEVICE_DRIVER_OFFSET` is the correct byte offset to the embedded `struct device_driver`.
 unsafe impl<T: Driver + 'static> driver::Driver for Adapter<T> {
     type DriverType = bindings::platform_driver;
+    type DriverData = T;
     const DEVICE_DRIVER_OFFSET: usize = core::mem::offset_of!(Self::DriverType, driver);
 }
 
diff --git a/rust/kernel/usb.rs b/rust/kernel/usb.rs
index a9a9d2298d87..b09fe8bcca13 100644
--- a/rust/kernel/usb.rs
+++ b/rust/kernel/usb.rs
@@ -29,10 +29,12 @@
 
 // SAFETY:
 // - `bindings::usb_driver` is a C type declared as `repr(C)`.
+// - `T` is the type of the driver's device private data.
 // - `struct usb_driver` embeds a `struct device_driver`.
 // - `DEVICE_DRIVER_OFFSET` is the correct byte offset to the embedded `struct device_driver`.
 unsafe impl<T: Driver + 'static> driver::Driver for Adapter<T> {
     type DriverType = bindings::usb_driver;
+    type DriverData = T;
     const DEVICE_DRIVER_OFFSET: usize = core::mem::offset_of!(Self::DriverType, driver);
 }
 
-- 
2.52.0

[PATCH 6/6] rust: driver: drop device private data post unbind

[Danilo Krummrich]

Currently, the driver's device private data is allocated and initialized
from driver core code called from bus abstractions after the driver's
probe() callback returned the corresponding initializer.

Similarly, the driver's device private data is dropped within the
remove() callback of bus abstractions after calling the remove()
callback of the corresponding driver.

However, commit 6f61a2637abe ("rust: device: introduce
Device::drvdata()") introduced an accessor for the driver's device
private data for a Device<Bound>, i.e. a device that is currently bound
to a driver.

Obviously, this is in conflict with dropping the driver's device private
data in remove(), since a device can not be considered to be fully
unbound after remove() has finished:

We also have to consider registrations guarded by devres - such as IRQ
or class device registrations - which are torn down after remove() in
devres_release_all().

Thus, it can happen that, for instance, a class device or IRQ callback
still calls Device::drvdata(), which then runs concurrently to remove()
(which sets dev->driver_data to NULL and drops the driver's device
private data), before devres_release_all() started to tear down the
corresponding registration. This is because devres guarded registrations
can, as expected, access the corresponding Device<Bound> that defines
their scope.

In C it simply is the driver's responsibility to ensure that its device
private data is freed after e.g. an IRQ registration is unregistered.

Typically, C drivers achieve this by allocating their device private data
with e.g. devm_kzalloc() before doing anything else, i.e. before e.g.
registering an IRQ with devm_request_threaded_irq(), relying on the
reverse order cleanup of devres.

Technically, we could do something similar in Rust. However, the
resulting code would be pretty messy:

In Rust we have to differentiate between allocated but uninitialized
memory and initialized memory in the type system. Thus, we would need to
somehow keep track of whether the driver's device private data object
has been initialized (i.e. probe() was successful and returned a valid
initializer for this memory) and conditionally call the destructor of
the corresponding object when it is freed.

This is because we'd need to allocate and register the memory of the
driver's device private data *before* it is initialized by the
initializer returned by the driver's probe() callback, because the
driver could already register devres guarded registrations within
probe() outside of the driver's device private data initializer.

Luckily there is a much simpler solution: Instead of dropping the
driver's device private data at the end of remove(), we just drop it
after the device has been fully unbound, i.e. after all devres callbacks
have been processed.

For this, we introduce a new post_unbind() callback private to the
driver-core, i.e. the callback is neither exposed to drivers, nor to bus
abstractions.

This way, the driver-core code can simply continue to conditionally
allocate the memory for the driver's device private data when the
driver's initializer is returned from probe() - no change needed - and
drop it when the driver-core code receives the post_unbind() callback.

Closes: https://lore.kernel.org/all/DEZMS6Y4A7XE.XE7EUBT5SJFJ@kernel.org/
Fixes: 6f61a2637abe ("rust: device: introduce Device::drvdata()")
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 drivers/base/dd.c             |  4 ++++
 include/linux/device/driver.h | 11 +++++++++++
 rust/kernel/auxiliary.rs      |  4 ++--
 rust/kernel/device.rs         | 20 ++++++++++---------
 rust/kernel/driver.rs         | 36 ++++++++++++++++++++++++++++++++++-
 rust/kernel/i2c.rs            |  4 ++--
 rust/kernel/pci.rs            |  4 ++--
 rust/kernel/platform.rs       |  4 ++--
 rust/kernel/usb.rs            |  4 ++--
 9 files changed, 71 insertions(+), 20 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 349f31bedfa1..2d9871503614 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -548,6 +548,10 @@ static DEVICE_ATTR_RW(state_synced);
 static void device_unbind_cleanup(struct device *dev)
 {
 	devres_release_all(dev);
+#ifdef CONFIG_RUST
+	if (dev->driver->p_cb.post_unbind)
+		dev->driver->p_cb.post_unbind(dev);
+#endif
 	arch_teardown_dma_ops(dev);
 	kfree(dev->dma_range_map);
 	dev->dma_range_map = NULL;
diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h
index cd8e0f0a634b..51a9ebdd8a2d 100644
--- a/include/linux/device/driver.h
+++ b/include/linux/device/driver.h
@@ -85,6 +85,8 @@ enum probe_type {
  *		uevent.
  * @p:		Driver core's private data, no one other than the driver
  *		core can touch this.
+ * @p_cb:	Callbacks private to the driver core; no one other than the
+ *		driver core is allowed to touch this.
  *
  * The device driver-model tracks all of the drivers known to the system.
  * The main reason for this tracking is to enable the driver core to match
@@ -119,6 +121,15 @@ struct device_driver {
 	void (*coredump) (struct device *dev);
 
 	struct driver_private *p;
+#ifdef CONFIG_RUST
+	struct {
+		/*
+		 * Called after remove() and after all devres entries have been
+		 * processed.
+		 */
+		void (*post_unbind)(struct device *dev);
+	} p_cb;
+#endif
 };
 
 
diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs
index cb26238e95b0..2b4a9aaabb65 100644
--- a/rust/kernel/auxiliary.rs
+++ b/rust/kernel/auxiliary.rs
@@ -96,9 +96,9 @@ extern "C" fn remove_callback(adev: *mut bindings::auxiliary_device) {
         // SAFETY: `remove_callback` is only ever called after a successful call to
         // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
         // and stored a `Pin<KBox<T>>`.
-        let data = unsafe { adev.as_ref().drvdata_obtain::<T>() };
+        let data = unsafe { adev.as_ref().drvdata_borrow::<T>() };
 
-        T::unbind(adev, data.as_ref());
+        T::unbind(adev, data);
     }
 }
 
diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 71b200df0f40..031720bf5d8c 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -232,30 +232,32 @@ pub fn set_drvdata<T: 'static>(&self, data: impl PinInit<T, Error>) -> Result {
     ///
     /// # 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: 'static>(&self) -> Pin<KBox<T>> {
+    pub(crate) unsafe fn drvdata_obtain<T: 'static>(&self) -> Option<Pin<KBox<T>>> {
         // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
         let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };
 
         // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
         unsafe { bindings::dev_set_drvdata(self.as_raw(), core::ptr::null_mut()) };
 
+        if ptr.is_null() {
+            return None;
+        }
+
         // SAFETY:
-        // - By the safety requirements of this function, `ptr` comes from a previous call to
-        //   `into_foreign()`.
+        // - If `ptr` is not NULL, it 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 { Pin::<KBox<T>>::from_foreign(ptr.cast()) }
+        Some(unsafe { Pin::<KBox<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`].
+    /// - Must only be called after a preceding call to [`Device::set_drvdata`] and before the
+    ///   device is fully unbound.
     /// - The type `T` must match the type of the `ForeignOwnable` previously stored by
     ///   [`Device::set_drvdata`].
     pub unsafe fn drvdata_borrow<T: 'static>(&self) -> Pin<&T> {
@@ -271,7 +273,7 @@ impl Device<Bound> {
     /// # Safety
     ///
     /// - Must only be called after a preceding call to [`Device::set_drvdata`] and before
-    ///   [`Device::drvdata_obtain`].
+    ///   the device is fully unbound.
     /// - The type `T` must match the type of the `ForeignOwnable` previously stored by
     ///   [`Device::set_drvdata`].
     unsafe fn drvdata_unchecked<T: 'static>(&self) -> Pin<&T> {
@@ -320,7 +322,7 @@ pub fn drvdata<T: 'static>(&self) -> Result<Pin<&T>> {
 
         // SAFETY:
         // - The above check of `dev_get_drvdata()` guarantees that we are called after
-        //   `set_drvdata()` and before `drvdata_obtain()`.
+        //   `set_drvdata()`.
         // - We've just checked that the type of the driver's private data is in fact `T`.
         Ok(unsafe { self.drvdata_unchecked() })
     }
diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs
index 77c1f7434897..6e32376d4c7c 100644
--- a/rust/kernel/driver.rs
+++ b/rust/kernel/driver.rs
@@ -177,7 +177,39 @@ unsafe impl<T: RegistrationOps> Sync for Registration<T> {}
 // any thread, so `Registration` is `Send`.
 unsafe impl<T: RegistrationOps> Send for Registration<T> {}
 
-impl<T: RegistrationOps> Registration<T> {
+impl<T: RegistrationOps + 'static> Registration<T> {
+    extern "C" fn post_unbind_callback(dev: *mut bindings::device) {
+        // SAFETY: The driver core only ever calls the post unbind callback with a valid pointer to
+        // a `struct device`.
+        //
+        // INVARIANT: `dev` is valid for the duration of the `post_unbind_callback()`.
+        let dev = unsafe { &*dev.cast::<device::Device<device::CoreInternal>>() };
+
+        // `remove()` and all devres callbacks have been completed at this point, hence drop the
+        // driver's device private data.
+        //
+        // SAFETY: By the safety requirements of the `Driver` trait, `T::DriverData` is the
+        // driver's device private data.
+        drop(unsafe { dev.drvdata_obtain::<T::DriverData>() });
+    }
+
+    /// Attach generic `struct device_driver` callbacks.
+    fn callbacks_attach(drv: &Opaque<T::DriverType>) {
+        let ptr = drv.get().cast::<u8>();
+
+        // SAFETY:
+        // - `drv.get()` yields a valid pointer to `Self::DriverType`.
+        // - Adding `DEVICE_DRIVER_OFFSET` yields the address of the embedded `struct device_driver`
+        //   as guaranteed by the safety requirements of the `Driver` trait.
+        let base = unsafe { ptr.add(T::DEVICE_DRIVER_OFFSET) };
+
+        // CAST: `base` points to the offset of the embedded `struct device_driver`.
+        let base = base.cast::<bindings::device_driver>();
+
+        // SAFETY: It is safe to set the fields of `struct device_driver` on initialization.
+        unsafe { (*base).p_cb.post_unbind = Some(Self::post_unbind_callback) };
+    }
+
     /// Creates a new instance of the registration object.
     pub fn new(name: &'static CStr, module: &'static ThisModule) -> impl PinInit<Self, Error> {
         try_pin_init!(Self {
@@ -189,6 +221,8 @@ pub fn new(name: &'static CStr, module: &'static ThisModule) -> impl PinInit<Sel
                 // just been initialised above, so it's also valid for read.
                 let drv = unsafe { &*(ptr as *const Opaque<T::DriverType>) };
 
+                Self::callbacks_attach(drv);
+
                 // SAFETY: `drv` is guaranteed to be pinned until `T::unregister`.
                 unsafe { T::register(drv, name, module) }
             }),
diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
index 6a3923a8b8a7..c1813f94da7a 100644
--- a/rust/kernel/i2c.rs
+++ b/rust/kernel/i2c.rs
@@ -178,9 +178,9 @@ extern "C" fn remove_callback(idev: *mut bindings::i2c_client) {
         // SAFETY: `remove_callback` is only ever called after a successful call to
         // `probe_callback`, hence it's guaranteed that `I2cClient::set_drvdata()` has been called
         // and stored a `Pin<KBox<T>>`.
-        let data = unsafe { idev.as_ref().drvdata_obtain::<T>() };
+        let data = unsafe { idev.as_ref().drvdata_borrow::<T>() };
 
-        T::unbind(idev, data.as_ref());
+        T::unbind(idev, data);
     }
 
     extern "C" fn shutdown_callback(idev: *mut bindings::i2c_client) {
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index fe63b53d55d6..8fdc18faeb4c 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -123,9 +123,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 data = unsafe { pdev.as_ref().drvdata_obtain::<T>() };
+        let data = unsafe { pdev.as_ref().drvdata_borrow::<T>() };
 
-        T::unbind(pdev, data.as_ref());
+        T::unbind(pdev, data);
     }
 }
 
diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index af94fb58aafb..6f81186a7919 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -101,9 +101,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 data = unsafe { pdev.as_ref().drvdata_obtain::<T>() };
+        let data = unsafe { pdev.as_ref().drvdata_borrow::<T>() };
 
-        T::unbind(pdev, data.as_ref());
+        T::unbind(pdev, data);
     }
 }
 
diff --git a/rust/kernel/usb.rs b/rust/kernel/usb.rs
index b09fe8bcca13..e6080955f742 100644
--- a/rust/kernel/usb.rs
+++ b/rust/kernel/usb.rs
@@ -103,9 +103,9 @@ extern "C" fn disconnect_callback(intf: *mut bindings::usb_interface) {
         // SAFETY: `disconnect_callback` is only ever called after a successful call to
         // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
         // and stored a `Pin<KBox<T>>`.
-        let data = unsafe { dev.drvdata_obtain::<T>() };
+        let data = unsafe { dev.drvdata_borrow::<T>() };
 
-        T::disconnect(intf, data.as_ref());
+        T::disconnect(intf, data);
     }
 }
 
-- 
2.52.0

Re: [PATCH 6/6] rust: driver: drop device private data post unbind

[Greg KH]

On Wed, Jan 07, 2026 at 11:35:05AM +0100, Danilo Krummrich wrote:
> @@ -548,6 +548,10 @@ static DEVICE_ATTR_RW(state_synced);
>  static void device_unbind_cleanup(struct device *dev)
>  {
>  	devres_release_all(dev);
> +#ifdef CONFIG_RUST

Nit, let's not put #ifdef in .c files, the overhead of an empty pointer
for all drivers is not a big deal.

> +	if (dev->driver->p_cb.post_unbind)
> +		dev->driver->p_cb.post_unbind(dev);
> +#endif
>  	arch_teardown_dma_ops(dev);
>  	kfree(dev->dma_range_map);
>  	dev->dma_range_map = NULL;
> diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h
> index cd8e0f0a634b..51a9ebdd8a2d 100644
> --- a/include/linux/device/driver.h
> +++ b/include/linux/device/driver.h
> @@ -85,6 +85,8 @@ enum probe_type {
>   *		uevent.
>   * @p:		Driver core's private data, no one other than the driver
>   *		core can touch this.
> + * @p_cb:	Callbacks private to the driver core; no one other than the
> + *		driver core is allowed to touch this.
>   *
>   * The device driver-model tracks all of the drivers known to the system.
>   * The main reason for this tracking is to enable the driver core to match
> @@ -119,6 +121,15 @@ struct device_driver {
>  	void (*coredump) (struct device *dev);
>  
>  	struct driver_private *p;
> +#ifdef CONFIG_RUST

Again, no #ifdef.

> +	struct {
> +		/*
> +		 * Called after remove() and after all devres entries have been
> +		 * processed.
> +		 */
> +		void (*post_unbind)(struct device *dev);

post_unbind_rust_only()?

> -impl<T: RegistrationOps> Registration<T> {
> +impl<T: RegistrationOps + 'static> Registration<T> {
> +    extern "C" fn post_unbind_callback(dev: *mut bindings::device) {
> +        // SAFETY: The driver core only ever calls the post unbind callback with a valid pointer to
> +        // a `struct device`.
> +        //
> +        // INVARIANT: `dev` is valid for the duration of the `post_unbind_callback()`.
> +        let dev = unsafe { &*dev.cast::<device::Device<device::CoreInternal>>() };
> +
> +        // `remove()` and all devres callbacks have been completed at this point, hence drop the
> +        // driver's device private data.
> +        //
> +        // SAFETY: By the safety requirements of the `Driver` trait, `T::DriverData` is the
> +        // driver's device private data.
> +        drop(unsafe { dev.drvdata_obtain::<T::DriverData>() });

I don't mind this, but why don't we also do this for all C drivers?
Just null out the pointer at this point in time so that no one can touch
it, just like you are doing here (in a way.)

thanks,

greg k-h

Re: [PATCH 6/6] rust: driver: drop device private data post unbind

[Danilo Krummrich]

On Wed Jan 7, 2026 at 1:22 PM CET, Greg KH wrote:
> On Wed, Jan 07, 2026 at 11:35:05AM +0100, Danilo Krummrich wrote:
>> @@ -548,6 +548,10 @@ static DEVICE_ATTR_RW(state_synced);
>>  static void device_unbind_cleanup(struct device *dev)
>>  {
>>  	devres_release_all(dev);
>> +#ifdef CONFIG_RUST
>
> Nit, let's not put #ifdef in .c files, the overhead of an empty pointer
> for all drivers is not a big deal.

I agree, I mainly did it to make it clear that, as by now, this is only used by
Rust driver-core code. However, ...

>> +	if (dev->driver->p_cb.post_unbind)
>> +		dev->driver->p_cb.post_unbind(dev);
>> +#endif

<snip>

>> +	struct {
>> +		/*
>> +		 * Called after remove() and after all devres entries have been
>> +		 * processed.
>> +		 */
>> +		void (*post_unbind)(struct device *dev);
>
> post_unbind_rust_only()?

...this works as well. We can always rename it, in case we start using it in C
too.

So, I'm fine with either. :)

>> -impl<T: RegistrationOps> Registration<T> {
>> +impl<T: RegistrationOps + 'static> Registration<T> {
>> +    extern "C" fn post_unbind_callback(dev: *mut bindings::device) {
>> +        // SAFETY: The driver core only ever calls the post unbind callback with a valid pointer to
>> +        // a `struct device`.
>> +        //
>> +        // INVARIANT: `dev` is valid for the duration of the `post_unbind_callback()`.
>> +        let dev = unsafe { &*dev.cast::<device::Device<device::CoreInternal>>() };
>> +
>> +        // `remove()` and all devres callbacks have been completed at this point, hence drop the
>> +        // driver's device private data.
>> +        //
>> +        // SAFETY: By the safety requirements of the `Driver` trait, `T::DriverData` is the
>> +        // driver's device private data.
>> +        drop(unsafe { dev.drvdata_obtain::<T::DriverData>() });
>
> I don't mind this, but why don't we also do this for all C drivers?

What exactly do you mean? Manage the lifetime of the device private data
commonly in driver-core code?

> Just null out the pointer at this point in time so that no one can touch
> it, just like you are doing here (in a way.)

I think device_unbind_cleanup() already calls dev_set_drvdata(dev, NULL) [1], so
technically we do not have to do it necessarily in Device::drvdata_obtain() as
well.

However, with Device::drvdata_obtain() we take back ownership of the
Pin<KBox<T>> stored in dev->driver_data, so it makes sense to null out the
pointer at exactly this point in time.

[1] https://elixir.bootlin.com/linux/v6.19-rc4/source/drivers/base/dd.c#L555

Re: [PATCH 6/6] rust: driver: drop device private data post unbind

[Greg KH]

On Wed, Jan 07, 2026 at 01:50:43PM +0100, Danilo Krummrich wrote:
> On Wed Jan 7, 2026 at 1:22 PM CET, Greg KH wrote:
> > On Wed, Jan 07, 2026 at 11:35:05AM +0100, Danilo Krummrich wrote:
> >> @@ -548,6 +548,10 @@ static DEVICE_ATTR_RW(state_synced);
> >>  static void device_unbind_cleanup(struct device *dev)
> >>  {
> >>  	devres_release_all(dev);
> >> +#ifdef CONFIG_RUST
> >
> > Nit, let's not put #ifdef in .c files, the overhead of an empty pointer
> > for all drivers is not a big deal.
> 
> I agree, I mainly did it to make it clear that, as by now, this is only used by
> Rust driver-core code. However, ...
> 
> >> +	if (dev->driver->p_cb.post_unbind)
> >> +		dev->driver->p_cb.post_unbind(dev);
> >> +#endif
> 
> <snip>
> 
> >> +	struct {
> >> +		/*
> >> +		 * Called after remove() and after all devres entries have been
> >> +		 * processed.
> >> +		 */
> >> +		void (*post_unbind)(struct device *dev);
> >
> > post_unbind_rust_only()?
> 
> ...this works as well. We can always rename it, in case we start using it in C
> too.
> 
> So, I'm fine with either. :)

I say name it with "rust_" and take out the #ifdef, that makes it
simpler/easier to understand.

> >> -impl<T: RegistrationOps> Registration<T> {
> >> +impl<T: RegistrationOps + 'static> Registration<T> {
> >> +    extern "C" fn post_unbind_callback(dev: *mut bindings::device) {
> >> +        // SAFETY: The driver core only ever calls the post unbind callback with a valid pointer to
> >> +        // a `struct device`.
> >> +        //
> >> +        // INVARIANT: `dev` is valid for the duration of the `post_unbind_callback()`.
> >> +        let dev = unsafe { &*dev.cast::<device::Device<device::CoreInternal>>() };
> >> +
> >> +        // `remove()` and all devres callbacks have been completed at this point, hence drop the
> >> +        // driver's device private data.
> >> +        //
> >> +        // SAFETY: By the safety requirements of the `Driver` trait, `T::DriverData` is the
> >> +        // driver's device private data.
> >> +        drop(unsafe { dev.drvdata_obtain::<T::DriverData>() });
> >
> > I don't mind this, but why don't we also do this for all C drivers?
> 
> What exactly do you mean? Manage the lifetime of the device private data
> commonly in driver-core code?
> 
> > Just null out the pointer at this point in time so that no one can touch
> > it, just like you are doing here (in a way.)
> 
> I think device_unbind_cleanup() already calls dev_set_drvdata(dev, NULL) [1], so
> technically we do not have to do it necessarily in Device::drvdata_obtain() as
> well.
> 
> However, with Device::drvdata_obtain() we take back ownership of the
> Pin<KBox<T>> stored in dev->driver_data, so it makes sense to null out the
> pointer at exactly this point in time.

Ok, no objection from me.

thanks,

greg k-h

Re: [PATCH 0/6] Address race condition with Device::drvdata()

[Alice Ryhl]

On Wed, Jan 07, 2026 at 11:34:59AM +0100, Danilo Krummrich wrote:
> Currently, the driver's device private data is allocated and initialized
> from driver core code called from bus abstractions after the driver's
> probe() callback returned the corresponding initializer.
> 
> Similarly, the driver's device private data is dropped within the
> remove() callback of bus abstractions after calling the remove()
> callback of the corresponding driver.
> 
> However, commit 6f61a2637abe ("rust: device: introduce
> Device::drvdata()") introduced an accessor for the driver's device
> private data for a Device<Bound>, i.e. a device that is currently bound
> to a driver.
> 
> Obviously, this is in conflict with dropping the driver's device private
> data in remove(), since a device can not be considered to be fully
> unbound after remove() has finished:
> 
> We also have to consider registrations guarded by devres - such as IRQ
> or class device registrations - which are torn down after remove() in
> devres_release_all().
> 
> Thus, it can happen that, for instance, a class device or IRQ callback
> still calls Device::drvdata(), which then runs concurrently to remove()
> (which sets dev->driver_data to NULL and drops the driver's device
> private data), before devres_release_all() started to tear down the
> corresponding registration. This is because devres guarded registrations
> can, as expected, access the corresponding Device<Bound> that defines
> their scope.
> 
> In C it simply is the driver's responsibility to ensure that its device
> private data is freed after e.g. an IRQ registration is unregistered.
> 
> Typically, C drivers achieve this by allocating their device private data
> with e.g. devm_kzalloc() before doing anything else, i.e. before e.g.
> registering an IRQ with devm_request_threaded_irq(), relying on the
> reverse order cleanup of devres [1].
> 
> Technically, we could do something similar in Rust. However, the
> resulting code would be pretty messy:
> 
> In Rust we have to differentiate between allocated but uninitialized
> memory and initialized memory in the type system. Thus, we would need to
> somehow keep track of whether the driver's device private data object
> has been initialized (i.e. probe() was successful and returned a valid
> initializer for this memory) and conditionally call the destructor of
> the corresponding object when it is freed.
> 
> This is because we'd need to allocate and register the memory of the
> driver's device private data *before* it is initialized by the
> initializer returned by the driver's probe() callback, because the
> driver could already register devres guarded registrations within
> probe() outside of the driver's device private data initializer.
> 
> Luckily there is a much simpler solution: Instead of dropping the
> driver's device private data at the end of remove(), we just drop it
> after the device has been fully unbound, i.e. after all devres callbacks
> have been processed.
> 
> For this, we introduce a new post_unbind() callback private to the
> driver-core, i.e. the callback is neither exposed to drivers, nor to bus
> abstractions.
> 
> This way, the driver-core code can simply continue to conditionally
> allocate the memory for the driver's device private data when the
> driver's initializer is returned from probe() - no change needed - and
> drop it when the driver-core code receives the post_unbind() callback.
> 
> --
> 
> Dependency wise we need a common Driver trait that describes the layout of a
> specific driver structure, such as struct pci_driver or struct platform_driver.
> Additional to this specific driver type (which was previously the associated
> type RegType of the RegistrationOps) it provides the offset to the embedded
> struct device_driver and the type of the driver's device private data.
> 
> This patch series contains two additional dependencies:
> 
>   (1) A fix for i2c::Driver::shutdown() to not free the driver's device
>       private data at all, which otherwise causes the exact same bug, and
>       is not necessary in the first place anyways.
> 
>   (2) Add the auxiliary::Driver::unbind() callback. Strictly speaking,
>       this is not a dependency, but without this patch the main fix of this
>       series leaves the remove() callback of the auxiliary bus
>       abstraction with either dead code or quite some code removed;
>       code that we would otherwise add back immediately afterwards.
> 
> --
> 
> [1] In fact, the cleanup ordering of devres is a separate challenge in
>     Rust, since it is technically unsound to rely on the driver to pick
>     the correct order. I am already working on a solution for this;
>     luckily this also has some synergies with optimizing the required
>     synchronize_rcu() calls required by the Rust Devres container
>     structure down to exactly one per driver unbind.

I don't think these are really separate problems. And I think this may
be the wrong fix.

If a &Device<Bound> lets you access a given value, then we must not
destroy that value until after the last &Device<Bound> has expired.

A &Device<Bound> lets you access the driver private data. And a
&Device<Bound> lets you access the contents of a Devres<T>.

Thus, the last &Device<Bound> must expire before we destroy driver
private data or values inside of Devres<T>. Etc.

What are sources of &Device<Bound> today?

* Most driver callbacks.
* IRQ callbacks.
* Workqueue callbacks. (In the future.)
* I think that's it ...?

Thus, we must call free_irq() before we destroy *the first* Devres<T>
resource or the driver private data.

Thus, we must ensure that driver callbacks are somehow synchronized to
exit before we destroy *the first* Devres<T> resource or the driver
private data.

One thing is that this means that using a Devres<_> container and
callback as the mechanism for calling free_irq() is not possible.

I'm thinking that we may need two domains of callbacks:

1. devm_early_*
2. DEVICE IS NOW CONSIDERED UNBOUND - there must no longer be any &Device<Bound> left
3. free driver private data
4. devm_*

And devm_early_*() would be a new set of callbacks where we can run
things such as free_irq(). It would use a separate DevresEarly<_>
container, for which &Device<Bound> does *not* let you peek inside. And
the usual Devres<_> is cleaned up in step 4, where it is legal to peek
inside given a &Device<Bound>.

We could potentially revoke Devres<_> containers during devm_early_*,
but actually destroy their contents in devm_*. This gives you a clean
mechanism to replace the per-Devres<_> synchronize_rcu() calls with a
single one. (By declaring step 2 must last at least one rcu grace
period.)

Not sure whether remove() should run before or after step (2). If it
runs before, then it does not need to be synchronized with other device
callbacks and can free the driver private data. If it runs after (2),
then we can't destroy driver private data in remove().

Alice

Re: [PATCH 0/6] Address race condition with Device::drvdata()

[Danilo Krummrich]

On Wed Jan 7, 2026 at 4:51 PM CET, Alice Ryhl wrote:
> If a &Device<Bound> lets you access a given value, then we must not
> destroy that value until after the last &Device<Bound> has expired.
>
> A &Device<Bound> lets you access the driver private data. And a
> &Device<Bound> lets you access the contents of a Devres<T>.
>
> Thus, the last &Device<Bound> must expire before we destroy driver
> private data or values inside of Devres<T>. Etc.

Yes, the last &Device<Bound> must expire before we destroy the device private
data. This is exactly what is achieved by this patch. The device private data is
destroyed after all devres callbacks have been processed, which guarantees that
there can't be any contexts left that provide a &Device<Bound>.

As for the values inside of a Devres<T>, this is exactly what I refer to in my
paragraph above talking about the unsoundness of the devres cleanup ordering in
Rust.

I also mention that I'm already working on a solution and it is in fact pretty
close to the solution you propose below, i.e. a generic mechanism to support
multiple devres domains (which I also see advantages for in C code).

As mentioned, this will also help with getting the required synchronize_rcu()
calls down to exactly one per device unbind.

Technically, we could utilize such a devres domain for dropping the device
private data, but there is no need to have a separate domain just for this, we
already have a distinct place for dropping and freeing the device private data
after the device has been fully unbound, which is much simpler than a separate
devres domain.

Now, you may argue we don't need a separate devres domain, and that we could use
the non-early devres domain. However, this would have the following implication:

In the destructor of the device private data, drivers could still try to use
device resources stored in the device private data through try_access(), which
may or may not succeed depending on whether the corresponding Devres<T>
containers are part of the device private data initializer or whether they have
been allocated separately.

Or in other words it would leave room for drivers to abuse this behavior.

Therefore, the desired order is:

  1. Driver::unbind() (A place for drivers to tear down the device;
     registrations are up - unless explicitly revoked by the driver (this is a
     semantic choice) - and device resources are accessible.)

  2. devm_early_* (Drop all devres guarded registrations.)

  3. No more &Device<Bound> left.

  4. devm_* (Drop all device resources.)

  5. No more device resources left.

  6. Drop and free device private data. (try_access() will never succeed in the
     destructor of the device private data.

- Danilo

Re: [PATCH 6/6] rust: driver: drop device private data post unbind

[Danilo Krummrich]

On Wed Jan 7, 2026 at 3:54 PM CET, Greg KH wrote:
> I say name it with "rust_" and take out the #ifdef, that makes it
> simpler/easier to understand.

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 2d9871503614..bea8da5f8a3a 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -548,10 +548,8 @@ static DEVICE_ATTR_RW(state_synced);
 static void device_unbind_cleanup(struct device *dev)
 {
        devres_release_all(dev);
-#ifdef CONFIG_RUST
-       if (dev->driver->p_cb.post_unbind)
-               dev->driver->p_cb.post_unbind(dev);
-#endif
+       if (dev->driver->p_cb.post_unbind_rust)
+               dev->driver->p_cb.post_unbind_rust(dev);
        arch_teardown_dma_ops(dev);
        kfree(dev->dma_range_map);
        dev->dma_range_map = NULL;
diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h
index 51a9ebdd8a2d..bbc67ec513ed 100644
--- a/include/linux/device/driver.h
+++ b/include/linux/device/driver.h
@@ -121,15 +121,13 @@ struct device_driver {
        void (*coredump) (struct device *dev);

        struct driver_private *p;
-#ifdef CONFIG_RUST
        struct {
                /*
                 * Called after remove() and after all devres entries have been
-                * processed.
+                * processed. This is a Rust only callback.
                 */
-               void (*post_unbind)(struct device *dev);
+               void (*post_unbind_rust)(struct device *dev);
        } p_cb;
-#endif
 };


diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs
index 6e32376d4c7c..26095d7bd0d9 100644
--- a/rust/kernel/driver.rs
+++ b/rust/kernel/driver.rs
@@ -207,7 +207,7 @@ fn callbacks_attach(drv: &Opaque<T::DriverType>) {
         let base = base.cast::<bindings::device_driver>();

         // SAFETY: It is safe to set the fields of `struct device_driver` on initialization.
-        unsafe { (*base).p_cb.post_unbind = Some(Self::post_unbind_callback) };
+        unsafe { (*base).p_cb.post_unbind_rust = Some(Self::post_unbind_callback) };
     }

     /// Creates a new instance of the registration object.

Re: [PATCH 6/6] rust: driver: drop device private data post unbind

[Greg KH]

On Mon, Jan 12, 2026 at 03:27:08PM +0100, Danilo Krummrich wrote:
> On Wed Jan 7, 2026 at 3:54 PM CET, Greg KH wrote:
> > I say name it with "rust_" and take out the #ifdef, that makes it
> > simpler/easier to understand.
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 2d9871503614..bea8da5f8a3a 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -548,10 +548,8 @@ static DEVICE_ATTR_RW(state_synced);
>  static void device_unbind_cleanup(struct device *dev)
>  {
>         devres_release_all(dev);
> -#ifdef CONFIG_RUST
> -       if (dev->driver->p_cb.post_unbind)
> -               dev->driver->p_cb.post_unbind(dev);
> -#endif
> +       if (dev->driver->p_cb.post_unbind_rust)
> +               dev->driver->p_cb.post_unbind_rust(dev);
>         arch_teardown_dma_ops(dev);
>         kfree(dev->dma_range_map);
>         dev->dma_range_map = NULL;
> diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h
> index 51a9ebdd8a2d..bbc67ec513ed 100644
> --- a/include/linux/device/driver.h
> +++ b/include/linux/device/driver.h
> @@ -121,15 +121,13 @@ struct device_driver {
>         void (*coredump) (struct device *dev);
> 
>         struct driver_private *p;
> -#ifdef CONFIG_RUST
>         struct {
>                 /*
>                  * Called after remove() and after all devres entries have been
> -                * processed.
> +                * processed. This is a Rust only callback.
>                  */
> -               void (*post_unbind)(struct device *dev);
> +               void (*post_unbind_rust)(struct device *dev);
>         } p_cb;
> -#endif
>  };
> 
> 
> diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs
> index 6e32376d4c7c..26095d7bd0d9 100644
> --- a/rust/kernel/driver.rs
> +++ b/rust/kernel/driver.rs
> @@ -207,7 +207,7 @@ fn callbacks_attach(drv: &Opaque<T::DriverType>) {
>          let base = base.cast::<bindings::device_driver>();
> 
>          // SAFETY: It is safe to set the fields of `struct device_driver` on initialization.
> -        unsafe { (*base).p_cb.post_unbind = Some(Self::post_unbind_callback) };
> +        unsafe { (*base).p_cb.post_unbind_rust = Some(Self::post_unbind_callback) };
>      }
> 
>      /// Creates a new instance of the registration object.

Looks good to me, thanks!

Re: [PATCH 0/6] Address race condition with Device::drvdata()

[Alice Ryhl]

On Wed, Jan 07, 2026 at 05:40:20PM +0100, Danilo Krummrich wrote:
> On Wed Jan 7, 2026 at 4:51 PM CET, Alice Ryhl wrote:
> > If a &Device<Bound> lets you access a given value, then we must not
> > destroy that value until after the last &Device<Bound> has expired.
> >
> > A &Device<Bound> lets you access the driver private data. And a
> > &Device<Bound> lets you access the contents of a Devres<T>.
> >
> > Thus, the last &Device<Bound> must expire before we destroy driver
> > private data or values inside of Devres<T>. Etc.
> 
> Yes, the last &Device<Bound> must expire before we destroy the device private
> data. This is exactly what is achieved by this patch. The device private data is
> destroyed after all devres callbacks have been processed, which guarantees that
> there can't be any contexts left that provide a &Device<Bound>.
> 
> As for the values inside of a Devres<T>, this is exactly what I refer to in my
> paragraph above talking about the unsoundness of the devres cleanup ordering in
> Rust.
> 
> I also mention that I'm already working on a solution and it is in fact pretty
> close to the solution you propose below, i.e. a generic mechanism to support
> multiple devres domains (which I also see advantages for in C code).
> 
> As mentioned, this will also help with getting the required synchronize_rcu()
> calls down to exactly one per device unbind.
> 
> Technically, we could utilize such a devres domain for dropping the device
> private data, but there is no need to have a separate domain just for this, we
> already have a distinct place for dropping and freeing the device private data
> after the device has been fully unbound, which is much simpler than a separate
> devres domain.
> 
> Now, you may argue we don't need a separate devres domain, and that we could use
> the non-early devres domain. However, this would have the following implication:
> 
> In the destructor of the device private data, drivers could still try to use
> device resources stored in the device private data through try_access(), which
> may or may not succeed depending on whether the corresponding Devres<T>
> containers are part of the device private data initializer or whether they have
> been allocated separately.
> 
> Or in other words it would leave room for drivers to abuse this behavior.
> 
> Therefore, the desired order is:
> 
>   1. Driver::unbind() (A place for drivers to tear down the device;
>      registrations are up - unless explicitly revoked by the driver (this is a
>      semantic choice) - and device resources are accessible.)
> 
>   2. devm_early_* (Drop all devres guarded registrations.)
> 
>   3. No more &Device<Bound> left.
> 
>   4. devm_* (Drop all device resources.)
> 
>   5. No more device resources left.
> 
>   6. Drop and free device private data. (try_access() will never succeed in the
>      destructor of the device private data.

so your private data is just the first devres resource ;)

Ok. I'm worried that when you fix Devres, you have to undo changes you
made here. But I guess that's not the end of the world (and maybe you
don't have to).

Concept SGTM. I have not yet reviewed patches in details, but

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

Alice

Re: [PATCH 0/6] Address race condition with Device::drvdata()

[Danilo Krummrich]

On Mon Jan 12, 2026 at 4:34 PM CET, Alice Ryhl wrote:
> On Wed, Jan 07, 2026 at 05:40:20PM +0100, Danilo Krummrich wrote:
>> On Wed Jan 7, 2026 at 4:51 PM CET, Alice Ryhl wrote:
>> > If a &Device<Bound> lets you access a given value, then we must not
>> > destroy that value until after the last &Device<Bound> has expired.
>> >
>> > A &Device<Bound> lets you access the driver private data. And a
>> > &Device<Bound> lets you access the contents of a Devres<T>.
>> >
>> > Thus, the last &Device<Bound> must expire before we destroy driver
>> > private data or values inside of Devres<T>. Etc.
>> 
>> Yes, the last &Device<Bound> must expire before we destroy the device private
>> data. This is exactly what is achieved by this patch. The device private data is
>> destroyed after all devres callbacks have been processed, which guarantees that
>> there can't be any contexts left that provide a &Device<Bound>.
>> 
>> As for the values inside of a Devres<T>, this is exactly what I refer to in my
>> paragraph above talking about the unsoundness of the devres cleanup ordering in
>> Rust.
>> 
>> I also mention that I'm already working on a solution and it is in fact pretty
>> close to the solution you propose below, i.e. a generic mechanism to support
>> multiple devres domains (which I also see advantages for in C code).
>> 
>> As mentioned, this will also help with getting the required synchronize_rcu()
>> calls down to exactly one per device unbind.
>> 
>> Technically, we could utilize such a devres domain for dropping the device
>> private data, but there is no need to have a separate domain just for this, we
>> already have a distinct place for dropping and freeing the device private data
>> after the device has been fully unbound, which is much simpler than a separate
>> devres domain.
>> 
>> Now, you may argue we don't need a separate devres domain, and that we could use
>> the non-early devres domain. However, this would have the following implication:
>> 
>> In the destructor of the device private data, drivers could still try to use
>> device resources stored in the device private data through try_access(), which
>> may or may not succeed depending on whether the corresponding Devres<T>
>> containers are part of the device private data initializer or whether they have
>> been allocated separately.
>> 
>> Or in other words it would leave room for drivers to abuse this behavior.
>> 
>> Therefore, the desired order is:
>> 
>>   1. Driver::unbind() (A place for drivers to tear down the device;
>>      registrations are up - unless explicitly revoked by the driver (this is a
>>      semantic choice) - and device resources are accessible.)
>> 
>>   2. devm_early_* (Drop all devres guarded registrations.)
>> 
>>   3. No more &Device<Bound> left.
>> 
>>   4. devm_* (Drop all device resources.)
>> 
>>   5. No more device resources left.
>> 
>>   6. Drop and free device private data. (try_access() will never succeed in the
>>      destructor of the device private data.
>
> so your private data is just the first devres resource ;)

Correct, that would work as well. However, I have a paragraph in the cover
letter explaining why this implementation is not desirable, i.e. more error
prone implementation and more explicit handling required by bus code.

> Ok. I'm worried that when you fix Devres, you have to undo changes you
> made here. But I guess that's not the end of the world (and maybe you
> don't have to).

For the reasons above this will remain as is even with the Devres rework. With a
separate devres stage it would become less error prone and we could also avoid
bus code involvement, but it would still be more complicated than a simple
callback.

> Concept SGTM. I have not yet reviewed patches in details, but
>
> Acked-by: Alice Ryhl <aliceryhl@google.com>

Thanks for taking a look! :)

Re: [PATCH 3/6] rust: driver: introduce a common Driver trait

[Igor Korotin]

On 1/7/2026 10:35 AM, Danilo Krummrich wrote:
> The Driver trait describes the layout of a specific driver structure,
> such as `struct pci_driver` or `struct platform_driver`.
> 
> In a first step, this replaces the associated type RegType of the
> RegistrationOps with the Driver::DriverType associated type.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>   rust/kernel/auxiliary.rs | 18 +++++++++++-------
>   rust/kernel/driver.rs    | 40 +++++++++++++++++++++++++---------------
>   rust/kernel/i2c.rs       | 18 +++++++++++-------
>   rust/kernel/pci.rs       | 18 +++++++++++-------
>   rust/kernel/platform.rs  | 18 +++++++++++-------
>   rust/kernel/usb.rs       | 18 +++++++++++-------
>   6 files changed, 80 insertions(+), 50 deletions(-)
> 
> diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs
> index 6931f8a4267f..4636b6f41195 100644
> --- a/rust/kernel/auxiliary.rs
> +++ b/rust/kernel/auxiliary.rs
> @@ -23,13 +23,17 @@
>   /// An adapter for the registration of auxiliary drivers.
>   pub struct Adapter<T: Driver>(T);
>   
> -// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if
> +// SAFETY:
> +// - `bindings::auxiliary_driver` is a C type declared as `repr(C)`.
> +unsafe impl<T: Driver + 'static> driver::Driver for Adapter<T> {
> +    type DriverType = bindings::auxiliary_driver;
> +}
> +
> +// SAFETY: A call to `unregister` for a given instance of `DriverType` is guaranteed to be valid if
>   // a preceding call to `register` has been successful.
>   unsafe impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
> -    type RegType = bindings::auxiliary_driver;
> -
>       unsafe fn register(
> -        adrv: &Opaque<Self::RegType>,
> +        adrv: &Opaque<Self::DriverType>,
>           name: &'static CStr,
>           module: &'static ThisModule,
>       ) -> Result {
> @@ -41,14 +45,14 @@ unsafe fn register(
>               (*adrv.get()).id_table = T::ID_TABLE.as_ptr();
>           }
>   
> -        // SAFETY: `adrv` is guaranteed to be a valid `RegType`.
> +        // SAFETY: `adrv` is guaranteed to be a valid `DriverType`.
>           to_result(unsafe {
>               bindings::__auxiliary_driver_register(adrv.get(), module.0, name.as_char_ptr())
>           })
>       }
>   
> -    unsafe fn unregister(adrv: &Opaque<Self::RegType>) {
> -        // SAFETY: `adrv` is guaranteed to be a valid `RegType`.
> +    unsafe fn unregister(adrv: &Opaque<Self::DriverType>) {
> +        // SAFETY: `adrv` is guaranteed to be a valid `DriverType`.
>           unsafe { bindings::auxiliary_driver_unregister(adrv.get()) }
>       }
>   }
> diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs
> index 649d06468f41..cd1d36c313e1 100644
> --- a/rust/kernel/driver.rs
> +++ b/rust/kernel/driver.rs
> @@ -99,23 +99,33 @@
>   use core::pin::Pin;
>   use pin_init::{pin_data, pinned_drop, PinInit};
>   
> +/// Trait describing the layout of a specific device driver.
> +///
> +/// This trait describes the layout of a specific driver structure, such as `struct pci_driver` or
> +/// `struct platform_driver`.
> +///
> +/// # Safety
> +///
> +/// Implementors must guarantee that:
> +/// - `DriverType` is `repr(C)`.
> +pub unsafe trait Driver {
> +    /// The specific driver type embedding a `struct device_driver`.
> +    type DriverType: Default;
> +}
> +
>   /// The [`RegistrationOps`] trait serves as generic interface for subsystems (e.g., PCI, Platform,
>   /// Amba, etc.) to provide the corresponding subsystem specific implementation to register /
> -/// unregister a driver of the particular type (`RegType`).
> +/// unregister a driver of the particular type (`DriverType`).
>   ///
> -/// For instance, the PCI subsystem would set `RegType` to `bindings::pci_driver` and call
> +/// For instance, the PCI subsystem would set `DriverType` to `bindings::pci_driver` and call
>   /// `bindings::__pci_register_driver` from `RegistrationOps::register` and
>   /// `bindings::pci_unregister_driver` from `RegistrationOps::unregister`.
>   ///
>   /// # Safety
>   ///
> -/// A call to [`RegistrationOps::unregister`] for a given instance of `RegType` is only valid if a
> -/// preceding call to [`RegistrationOps::register`] has been successful.
> -pub unsafe trait RegistrationOps {
> -    /// The type that holds information about the registration. This is typically a struct defined
> -    /// by the C portion of the kernel.
> -    type RegType: Default;
> -
> +/// A call to [`RegistrationOps::unregister`] for a given instance of `DriverType` is only valid if
> +/// a preceding call to [`RegistrationOps::register`] has been successful.
> +pub unsafe trait RegistrationOps: Driver {
>       /// Registers a driver.
>       ///
>       /// # Safety
> @@ -123,7 +133,7 @@ pub unsafe trait RegistrationOps {
>       /// On success, `reg` must remain pinned and valid until the matching call to
>       /// [`RegistrationOps::unregister`].
>       unsafe fn register(
> -        reg: &Opaque<Self::RegType>,
> +        reg: &Opaque<Self::DriverType>,
>           name: &'static CStr,
>           module: &'static ThisModule,
>       ) -> Result;
> @@ -134,7 +144,7 @@ unsafe fn register(
>       ///
>       /// Must only be called after a preceding successful call to [`RegistrationOps::register`] for
>       /// the same `reg`.
> -    unsafe fn unregister(reg: &Opaque<Self::RegType>);
> +    unsafe fn unregister(reg: &Opaque<Self::DriverType>);
>   }
>   
>   /// A [`Registration`] is a generic type that represents the registration of some driver type (e.g.
> @@ -146,7 +156,7 @@ unsafe fn register(
>   #[pin_data(PinnedDrop)]
>   pub struct Registration<T: RegistrationOps> {
>       #[pin]
> -    reg: Opaque<T::RegType>,
> +    reg: Opaque<T::DriverType>,
>   }
>   
>   // SAFETY: `Registration` has no fields or methods accessible via `&Registration`, so it is safe to
> @@ -161,13 +171,13 @@ impl<T: RegistrationOps> Registration<T> {
>       /// Creates a new instance of the registration object.
>       pub fn new(name: &'static CStr, module: &'static ThisModule) -> impl PinInit<Self, Error> {
>           try_pin_init!(Self {
> -            reg <- Opaque::try_ffi_init(|ptr: *mut T::RegType| {
> +            reg <- Opaque::try_ffi_init(|ptr: *mut T::DriverType| {
>                   // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write.
> -                unsafe { ptr.write(T::RegType::default()) };
> +                unsafe { ptr.write(T::DriverType::default()) };
>   
>                   // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write, and it has
>                   // just been initialised above, so it's also valid for read.
> -                let drv = unsafe { &*(ptr as *const Opaque<T::RegType>) };
> +                let drv = unsafe { &*(ptr as *const Opaque<T::DriverType>) };
>   
>                   // SAFETY: `drv` is guaranteed to be pinned until `T::unregister`.
>                   unsafe { T::register(drv, name, module) }
> diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
> index 35b678b78d91..de35961c6903 100644
> --- a/rust/kernel/i2c.rs
> +++ b/rust/kernel/i2c.rs
> @@ -92,13 +92,17 @@ macro_rules! i2c_device_table {
>   /// An adapter for the registration of I2C drivers.
>   pub struct Adapter<T: Driver>(T);
>   
> -// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if
> +// SAFETY:
> +// - `bindings::i2c_driver` is a C type declared as `repr(C)`.
> +unsafe impl<T: Driver + 'static> driver::Driver for Adapter<T> {
> +    type DriverType = bindings::i2c_driver;
> +}
> +
> +// SAFETY: A call to `unregister` for a given instance of `DriverType` is guaranteed to be valid if
>   // a preceding call to `register` has been successful.
>   unsafe impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
> -    type RegType = bindings::i2c_driver;
> -
>       unsafe fn register(
> -        idrv: &Opaque<Self::RegType>,
> +        idrv: &Opaque<Self::DriverType>,
>           name: &'static CStr,
>           module: &'static ThisModule,
>       ) -> Result {
> @@ -133,12 +137,12 @@ unsafe fn register(
>               (*idrv.get()).driver.acpi_match_table = acpi_table;
>           }
>   
> -        // SAFETY: `idrv` is guaranteed to be a valid `RegType`.
> +        // SAFETY: `idrv` is guaranteed to be a valid `DriverType`.
>           to_result(unsafe { bindings::i2c_register_driver(module.0, idrv.get()) })
>       }
>   
> -    unsafe fn unregister(idrv: &Opaque<Self::RegType>) {
> -        // SAFETY: `idrv` is guaranteed to be a valid `RegType`.
> +    unsafe fn unregister(idrv: &Opaque<Self::DriverType>) {
> +        // SAFETY: `idrv` is guaranteed to be a valid `DriverType`.
>           unsafe { bindings::i2c_del_driver(idrv.get()) }
>       }
>   }
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 82e128431f08..f58ce35d9c60 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -50,13 +50,17 @@
>   /// An adapter for the registration of PCI drivers.
>   pub struct Adapter<T: Driver>(T);
>   
> -// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if
> +// SAFETY:
> +// - `bindings::pci_driver` is a C type declared as `repr(C)`.
> +unsafe impl<T: Driver + 'static> driver::Driver for Adapter<T> {
> +    type DriverType = bindings::pci_driver;
> +}
> +
> +// SAFETY: A call to `unregister` for a given instance of `DriverType` is guaranteed to be valid if
>   // a preceding call to `register` has been successful.
>   unsafe impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
> -    type RegType = bindings::pci_driver;
> -
>       unsafe fn register(
> -        pdrv: &Opaque<Self::RegType>,
> +        pdrv: &Opaque<Self::DriverType>,
>           name: &'static CStr,
>           module: &'static ThisModule,
>       ) -> Result {
> @@ -68,14 +72,14 @@ unsafe fn register(
>               (*pdrv.get()).id_table = T::ID_TABLE.as_ptr();
>           }
>   
> -        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
> +        // SAFETY: `pdrv` is guaranteed to be a valid `DriverType`.
>           to_result(unsafe {
>               bindings::__pci_register_driver(pdrv.get(), module.0, name.as_char_ptr())
>           })
>       }
>   
> -    unsafe fn unregister(pdrv: &Opaque<Self::RegType>) {
> -        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
> +    unsafe fn unregister(pdrv: &Opaque<Self::DriverType>) {
> +        // SAFETY: `pdrv` is guaranteed to be a valid `DriverType`.
>           unsafe { bindings::pci_unregister_driver(pdrv.get()) }
>       }
>   }
> diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
> index ed889f079cab..e48d055fdc8a 100644
> --- a/rust/kernel/platform.rs
> +++ b/rust/kernel/platform.rs
> @@ -26,13 +26,17 @@
>   /// An adapter for the registration of platform drivers.
>   pub struct Adapter<T: Driver>(T);
>   
> -// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if
> +// SAFETY:
> +// - `bindings::platform_driver` is a C type declared as `repr(C)`.
> +unsafe impl<T: Driver + 'static> driver::Driver for Adapter<T> {
> +    type DriverType = bindings::platform_driver;
> +}
> +
> +// SAFETY: A call to `unregister` for a given instance of `DriverType` is guaranteed to be valid if
>   // a preceding call to `register` has been successful.
>   unsafe impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
> -    type RegType = bindings::platform_driver;
> -
>       unsafe fn register(
> -        pdrv: &Opaque<Self::RegType>,
> +        pdrv: &Opaque<Self::DriverType>,
>           name: &'static CStr,
>           module: &'static ThisModule,
>       ) -> Result {
> @@ -55,12 +59,12 @@ unsafe fn register(
>               (*pdrv.get()).driver.acpi_match_table = acpi_table;
>           }
>   
> -        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
> +        // SAFETY: `pdrv` is guaranteed to be a valid `DriverType`.
>           to_result(unsafe { bindings::__platform_driver_register(pdrv.get(), module.0) })
>       }
>   
> -    unsafe fn unregister(pdrv: &Opaque<Self::RegType>) {
> -        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
> +    unsafe fn unregister(pdrv: &Opaque<Self::DriverType>) {
> +        // SAFETY: `pdrv` is guaranteed to be a valid `DriverType`.
>           unsafe { bindings::platform_driver_unregister(pdrv.get()) };
>       }
>   }
> diff --git a/rust/kernel/usb.rs b/rust/kernel/usb.rs
> index d10b65e9fb6a..32f4b2d55dfb 100644
> --- a/rust/kernel/usb.rs
> +++ b/rust/kernel/usb.rs
> @@ -27,13 +27,17 @@
>   /// An adapter for the registration of USB drivers.
>   pub struct Adapter<T: Driver>(T);
>   
> -// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if
> +// SAFETY:
> +// - `bindings::usb_driver` is a C type declared as `repr(C)`.
> +unsafe impl<T: Driver + 'static> driver::Driver for Adapter<T> {
> +    type DriverType = bindings::usb_driver;
> +}
> +
> +// SAFETY: A call to `unregister` for a given instance of `DriverType` is guaranteed to be valid if
>   // a preceding call to `register` has been successful.
>   unsafe impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
> -    type RegType = bindings::usb_driver;
> -
>       unsafe fn register(
> -        udrv: &Opaque<Self::RegType>,
> +        udrv: &Opaque<Self::DriverType>,
>           name: &'static CStr,
>           module: &'static ThisModule,
>       ) -> Result {
> @@ -45,14 +49,14 @@ unsafe fn register(
>               (*udrv.get()).id_table = T::ID_TABLE.as_ptr();
>           }
>   
> -        // SAFETY: `udrv` is guaranteed to be a valid `RegType`.
> +        // SAFETY: `udrv` is guaranteed to be a valid `DriverType`.
>           to_result(unsafe {
>               bindings::usb_register_driver(udrv.get(), module.0, name.as_char_ptr())
>           })
>       }
>   
> -    unsafe fn unregister(udrv: &Opaque<Self::RegType>) {
> -        // SAFETY: `udrv` is guaranteed to be a valid `RegType`.
> +    unsafe fn unregister(udrv: &Opaque<Self::DriverType>) {
> +        // SAFETY: `udrv` is guaranteed to be a valid `DriverType`.
>           unsafe { bindings::usb_deregister(udrv.get()) };
>       }
>   }

Acked-by: Igor Korotin <igor.korotin.linux@gmail.com>

Cheers
Igor

Re: [PATCH 0/6] Address race condition with Device::drvdata()

[Igor Korotin]

On 1/7/2026 10:34 AM, Danilo Krummrich wrote:
> Currently, the driver's device private data is allocated and initialized
> from driver core code called from bus abstractions after the driver's
> probe() callback returned the corresponding initializer.
> 
> Similarly, the driver's device private data is dropped within the
> remove() callback of bus abstractions after calling the remove()
> callback of the corresponding driver.
> 
> However, commit 6f61a2637abe ("rust: device: introduce
> Device::drvdata()") introduced an accessor for the driver's device
> private data for a Device<Bound>, i.e. a device that is currently bound
> to a driver.
> 
> Obviously, this is in conflict with dropping the driver's device private
> data in remove(), since a device can not be considered to be fully
> unbound after remove() has finished:
> 
> We also have to consider registrations guarded by devres - such as IRQ
> or class device registrations - which are torn down after remove() in
> devres_release_all().
> 
> Thus, it can happen that, for instance, a class device or IRQ callback
> still calls Device::drvdata(), which then runs concurrently to remove()
> (which sets dev->driver_data to NULL and drops the driver's device
> private data), before devres_release_all() started to tear down the
> corresponding registration. This is because devres guarded registrations
> can, as expected, access the corresponding Device<Bound> that defines
> their scope.
> 
> In C it simply is the driver's responsibility to ensure that its device
> private data is freed after e.g. an IRQ registration is unregistered.
> 
> Typically, C drivers achieve this by allocating their device private data
> with e.g. devm_kzalloc() before doing anything else, i.e. before e.g.
> registering an IRQ with devm_request_threaded_irq(), relying on the
> reverse order cleanup of devres [1].
> 
> Technically, we could do something similar in Rust. However, the
> resulting code would be pretty messy:
> 
> In Rust we have to differentiate between allocated but uninitialized
> memory and initialized memory in the type system. Thus, we would need to
> somehow keep track of whether the driver's device private data object
> has been initialized (i.e. probe() was successful and returned a valid
> initializer for this memory) and conditionally call the destructor of
> the corresponding object when it is freed.
> 
> This is because we'd need to allocate and register the memory of the
> driver's device private data *before* it is initialized by the
> initializer returned by the driver's probe() callback, because the
> driver could already register devres guarded registrations within
> probe() outside of the driver's device private data initializer.
> 
> Luckily there is a much simpler solution: Instead of dropping the
> driver's device private data at the end of remove(), we just drop it
> after the device has been fully unbound, i.e. after all devres callbacks
> have been processed.
> 
> For this, we introduce a new post_unbind() callback private to the
> driver-core, i.e. the callback is neither exposed to drivers, nor to bus
> abstractions.
> 
> This way, the driver-core code can simply continue to conditionally
> allocate the memory for the driver's device private data when the
> driver's initializer is returned from probe() - no change needed - and
> drop it when the driver-core code receives the post_unbind() callback.
> 
> --
> 
> Dependency wise we need a common Driver trait that describes the layout of a
> specific driver structure, such as struct pci_driver or struct platform_driver.
> Additional to this specific driver type (which was previously the associated
> type RegType of the RegistrationOps) it provides the offset to the embedded
> struct device_driver and the type of the driver's device private data.
> 
> This patch series contains two additional dependencies:
> 
>    (1) A fix for i2c::Driver::shutdown() to not free the driver's device
>        private data at all, which otherwise causes the exact same bug, and
>        is not necessary in the first place anyways.
> 
>    (2) Add the auxiliary::Driver::unbind() callback. Strictly speaking,
>        this is not a dependency, but without this patch the main fix of this
>        series leaves the remove() callback of the auxiliary bus
>        abstraction with either dead code or quite some code removed;
>        code that we would otherwise add back immediately afterwards.
> 
> --
> 
> [1] In fact, the cleanup ordering of devres is a separate challenge in
>      Rust, since it is technically unsound to rely on the driver to pick
>      the correct order. I am already working on a solution for this;
>      luckily this also has some synergies with optimizing the required
>      synchronize_rcu() calls required by the Rust Devres container
>      structure down to exactly one per driver unbind.
> 
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=driver/post_unbind
> 
> Danilo Krummrich (6):
>    rust: i2c: do not drop device private data on shutdown()
>    rust: auxiliary: add Driver::unbind() callback
>    rust: driver: introduce a common Driver trait
>    rust: driver: add DEVICE_DRIVER_OFFSET to the Driver trait
>    rust: driver: add DriverData type to the generic Driver trait
>    rust: driver: drop device private data post unbind
> 
>   drivers/base/dd.c             |  4 ++
>   include/linux/device/driver.h | 11 +++++
>   rust/kernel/auxiliary.rs      | 41 +++++++++++++----
>   rust/kernel/device.rs         | 20 ++++----
>   rust/kernel/driver.rs         | 86 ++++++++++++++++++++++++++++-------
>   rust/kernel/i2c.rs            | 31 ++++++++-----
>   rust/kernel/pci.rs            | 27 +++++++----
>   rust/kernel/platform.rs       | 27 +++++++----
>   rust/kernel/usb.rs            | 27 +++++++----
>   9 files changed, 203 insertions(+), 71 deletions(-)
> 
> 
> base-commit: 8510ef5e3cfbd7d59a16845f85cd0194a8689761

For the I2C parts: Acked-by: Igor Korotin <igor.korotin.linux@gmail.com>

Thanks
Igor

Re: [PATCH 0/6] Address race condition with Device::drvdata()

[Danilo Krummrich]

On Wed Jan 7, 2026 at 11:34 AM CET, Danilo Krummrich wrote:
> Danilo Krummrich (6):
>   rust: i2c: do not drop device private data on shutdown()
>   rust: auxiliary: add Driver::unbind() callback
>   rust: driver: introduce a common Driver trait

    [ Rename driver::Driver to driver::DriverLayout, as it represents the
      layout of a driver structure rather than the driver structure itself.
      - Danilo ]

>   rust: driver: add DEVICE_DRIVER_OFFSET to the Driver trait
>   rust: driver: add DriverData type to the generic Driver trait
>   rust: driver: drop device private data post unbind

    [ Remove #ifdef CONFIG_RUST, rename post_unbind() to post_unbind_rust().
     - Danilo]

Applied to driver-core-linus, thanks!