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

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

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

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/helpers/device.c | 10 ++++++++++
 rust/kernel/device.rs | 43 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/rust/helpers/device.c b/rust/helpers/device.c
index b2135c6686b0..9bf252649c75 100644
--- a/rust/helpers/device.c
+++ b/rust/helpers/device.c
@@ -8,3 +8,13 @@ int rust_helper_devm_add_action(struct device *dev,
 {
 	return devm_add_action(dev, action, data);
 }
+
+void *rust_helper_dev_get_drvdata(const struct device *dev)
+{
+	return dev_get_drvdata(dev);
+}
+
+void rust_helper_dev_set_drvdata(struct device *dev, void *data)
+{
+	dev_set_drvdata(dev, data);
+}
diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index e9094d8322d5..146eba147d2f 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -6,7 +6,7 @@
 
 use crate::{
     bindings,
-    types::{ARef, Opaque},
+    types::{ARef, ForeignOwnable, Opaque},
 };
 use core::{fmt, marker::PhantomData, ptr};
 
@@ -62,6 +62,47 @@ pub unsafe fn get_device(ptr: *mut bindings::device) -> ARef<Self> {
     }
 }
 
+impl Device<Internal> {
+    /// Store a pointer to the bound driver's private data.
+    pub fn set_drvdata(&self, data: impl ForeignOwnable) {
+        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
+        unsafe { bindings::dev_set_drvdata(self.as_raw(), data.into_foreign().cast()) }
+    }
+
+    /// Take ownership of the private data stored in this [`Device`].
+    ///
+    /// # Safety
+    ///
+    /// - Must only be called once after a preceding call to [`Device::set_drvdata`].
+    /// - The type `T` must match the type of the `ForeignOwnable` previously stored by
+    ///   [`Device::set_drvdata`].
+    pub unsafe fn drvdata_obtain<T: ForeignOwnable>(&self) -> T {
+        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
+        let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };
+
+        // SAFETY: By the safety requirements of this function, `ptr` comes from a previous call to
+        // `into_foreign()`.
+        unsafe { T::from_foreign(ptr.cast()) }
+    }
+
+    /// Borrow the driver's private data bound to this [`Device`].
+    ///
+    /// # Safety
+    ///
+    /// - Must only be called after a preceding call to [`Device::set_drvdata`] and before
+    ///   [`Device::drvdata_obtain`].
+    /// - The type `T` must match the type of the `ForeignOwnable` previously stored by
+    ///   [`Device::set_drvdata`].
+    pub unsafe fn drvdata_borrow<T: ForeignOwnable>(&self) -> T::Borrowed<'_> {
+        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
+        let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };
+
+        // SAFETY: By the safety requirements of this function, `ptr` comes from a previous call to
+        // `into_foreign()`.
+        unsafe { T::borrow(ptr.cast()) }
+    }
+}
+
 impl<Ctx: DeviceContext> Device<Ctx> {
     /// Obtain the raw `struct device *`.
     pub(crate) fn as_raw(&self) -> *mut bindings::device {
-- 
2.49.0

On Sun Jun 22, 2025 at 4:43 AM JST, Danilo Krummrich wrote:
<snip>
> +impl Device<Internal> {
> +    /// Store a pointer to the bound driver's private data.
> +    pub fn set_drvdata(&self, data: impl ForeignOwnable) {
> +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> +        unsafe { bindings::dev_set_drvdata(self.as_raw(), data.into_foreign().cast()) }
> +    }
> +
> +    /// Take ownership of the private data stored in this [`Device`].
> +    ///
> +    /// # Safety
> +    ///
> +    /// - Must only be called once after a preceding call to [`Device::set_drvdata`].
> +    /// - The type `T` must match the type of the `ForeignOwnable` previously stored by
> +    ///   [`Device::set_drvdata`].
> +    pub unsafe fn drvdata_obtain<T: ForeignOwnable>(&self) -> T {
> +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> +        let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };
> +
> +        // SAFETY: By the safety requirements of this function, `ptr` comes from a previous call to
> +        // `into_foreign()`.
> +        unsafe { T::from_foreign(ptr.cast()) }
> +    }
> +
> +    /// Borrow the driver's private data bound to this [`Device`].
> +    ///
> +    /// # Safety
> +    ///
> +    /// - Must only be called after a preceding call to [`Device::set_drvdata`] and before
> +    ///   [`Device::drvdata_obtain`].
> +    /// - The type `T` must match the type of the `ForeignOwnable` previously stored by
> +    ///   [`Device::set_drvdata`].
> +    pub unsafe fn drvdata_borrow<T: ForeignOwnable>(&self) -> T::Borrowed<'_> {
> +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> +        let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };
> +
> +        // SAFETY: By the safety requirements of this function, `ptr` comes from a previous call to
> +        // `into_foreign()`.
> +        unsafe { T::borrow(ptr.cast()) }
> +    }
> +}

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

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

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

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

You would do:

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

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

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

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

On Mon, Jul 07, 2025 at 04:46:09PM +0900, Alexandre Courbot wrote:
>     let pdev = unsafe { &*pdev.cast::<Device<InternalSet<Pin<KBox<T>>>>>() };
>     ...
>     // The type of the driver data is already known from `pdev`'s type,
>     // so this can be safe.
>     let foo = pdev.as_ref().drvdata_borrow();

I think this doesn't remove the safety requirement. drvdata_borrow() or
drvdata_obtain() would still require CoreInternal<Pin<KBox<T>>> to have the
correct type generic.

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

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

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

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

On Sat Jun 21, 2025 at 9:43 PM CEST, Danilo Krummrich wrote:
> +impl Device<Internal> {
> +    /// Store a pointer to the bound driver's private data.
> +    pub fn set_drvdata(&self, data: impl ForeignOwnable) {
> +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> +        unsafe { bindings::dev_set_drvdata(self.as_raw(), data.into_foreign().cast()) }
> +    }
> +
> +    /// Take ownership of the private data stored in this [`Device`].
> +    ///
> +    /// # Safety
> +    ///
> +    /// - Must only be called once after a preceding call to [`Device::set_drvdata`].
> +    /// - The type `T` must match the type of the `ForeignOwnable` previously stored by
> +    ///   [`Device::set_drvdata`].
> +    pub unsafe fn drvdata_obtain<T: ForeignOwnable>(&self) -> T {
> +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> +        let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };
> +
> +        // SAFETY: By the safety requirements of this function, `ptr` comes from a previous call to
> +        // `into_foreign()`.

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

Otherwise the safety docs look fine.

---
Cheers,
Benno

> +        unsafe { T::from_foreign(ptr.cast()) }
> +    }
> +
> +    /// Borrow the driver's private data bound to this [`Device`].
> +    ///
> +    /// # Safety
> +    ///
> +    /// - Must only be called after a preceding call to [`Device::set_drvdata`] and before
> +    ///   [`Device::drvdata_obtain`].
> +    /// - The type `T` must match the type of the `ForeignOwnable` previously stored by
> +    ///   [`Device::set_drvdata`].
> +    pub unsafe fn drvdata_borrow<T: ForeignOwnable>(&self) -> T::Borrowed<'_> {
> +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> +        let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };
> +
> +        // SAFETY: By the safety requirements of this function, `ptr` comes from a previous call to
> +        // `into_foreign()`.
> +        unsafe { T::borrow(ptr.cast()) }
> +    }
> +}
> +
>  impl<Ctx: DeviceContext> Device<Ctx> {
>      /// Obtain the raw `struct device *`.
>      pub(crate) fn as_raw(&self) -> *mut bindings::device {

On Sat, Jul 05, 2025 at 01:15:06PM +0200, Benno Lossin wrote:
> On Sat Jun 21, 2025 at 9:43 PM CEST, Danilo Krummrich wrote:
> > +impl Device<Internal> {
> > +    /// Store a pointer to the bound driver's private data.
> > +    pub fn set_drvdata(&self, data: impl ForeignOwnable) {
> > +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> > +        unsafe { bindings::dev_set_drvdata(self.as_raw(), data.into_foreign().cast()) }
> > +    }
> > +
> > +    /// Take ownership of the private data stored in this [`Device`].
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// - Must only be called once after a preceding call to [`Device::set_drvdata`].
> > +    /// - The type `T` must match the type of the `ForeignOwnable` previously stored by
> > +    ///   [`Device::set_drvdata`].
> > +    pub unsafe fn drvdata_obtain<T: ForeignOwnable>(&self) -> T {
> > +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> > +        let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };
> > +
> > +        // SAFETY: By the safety requirements of this function, `ptr` comes from a previous call to
> > +        // `into_foreign()`.
> 
> Well, you're also relying on `dev_get_drvdata` to return the same
> pointer that was given to `dev_set_drvdata`.
> 
> Otherwise the safety docs look fine.

Great! What do you think about:

diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 146eba147d2f..b01cb8e8dab3 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -80,8 +80,11 @@ pub unsafe fn drvdata_obtain<T: ForeignOwnable>(&self) -> T {
         // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
         let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };

-        // SAFETY: By the safety requirements of this function, `ptr` comes from a previous call to
-        // `into_foreign()`.
+        // SAFETY:
+        // - By the safety requirements of this function, `ptr` comes from a previous call to
+        //   `into_foreign()`.
+        // - `dev_get_drvdata()` guarantees to return the same pointer given to `dev_set_drvdata()`
+        //   in `into_foreign()`.
         unsafe { T::from_foreign(ptr.cast()) }
     }

On Sat Jul 5, 2025 at 5:06 PM CEST, Danilo Krummrich wrote:
> On Sat, Jul 05, 2025 at 01:15:06PM +0200, Benno Lossin wrote:
>> On Sat Jun 21, 2025 at 9:43 PM CEST, Danilo Krummrich wrote:
>> > +impl Device<Internal> {
>> > +    /// Store a pointer to the bound driver's private data.
>> > +    pub fn set_drvdata(&self, data: impl ForeignOwnable) {
>> > +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
>> > +        unsafe { bindings::dev_set_drvdata(self.as_raw(), data.into_foreign().cast()) }
>> > +    }
>> > +
>> > +    /// Take ownership of the private data stored in this [`Device`].
>> > +    ///
>> > +    /// # Safety
>> > +    ///
>> > +    /// - Must only be called once after a preceding call to [`Device::set_drvdata`].
>> > +    /// - The type `T` must match the type of the `ForeignOwnable` previously stored by
>> > +    ///   [`Device::set_drvdata`].
>> > +    pub unsafe fn drvdata_obtain<T: ForeignOwnable>(&self) -> T {
>> > +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
>> > +        let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };
>> > +
>> > +        // SAFETY: By the safety requirements of this function, `ptr` comes from a previous call to
>> > +        // `into_foreign()`.
>> 
>> Well, you're also relying on `dev_get_drvdata` to return the same
>> pointer that was given to `dev_set_drvdata`.
>> 
>> Otherwise the safety docs look fine.
>
> Great! What do you think about:
>
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index 146eba147d2f..b01cb8e8dab3 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -80,8 +80,11 @@ pub unsafe fn drvdata_obtain<T: ForeignOwnable>(&self) -> T {
>          // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
>          let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };
>
> -        // SAFETY: By the safety requirements of this function, `ptr` comes from a previous call to
> -        // `into_foreign()`.
> +        // SAFETY:
> +        // - By the safety requirements of this function, `ptr` comes from a previous call to
> +        //   `into_foreign()`.
> +        // - `dev_get_drvdata()` guarantees to return the same pointer given to `dev_set_drvdata()`
> +        //   in `into_foreign()`.

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

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

---
Cheers,
Benno

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

On Sat, Jun 21, 2025 at 09:43:28PM +0200, Danilo Krummrich wrote:
> Implement generic accessors for the private data of a driver bound to a
> device.
> 
> Those accessors should be used by bus abstractions from their
> corresponding core callbacks, such as probe(), remove(), etc.
> 
> Implementing them for device::Internal guarantees that driver's can't
> interfere with the logic implemented by the bus abstraction.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/helpers/device.c | 10 ++++++++++
>  rust/kernel/device.rs | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/helpers/device.c b/rust/helpers/device.c
> index b2135c6686b0..9bf252649c75 100644
> --- a/rust/helpers/device.c
> +++ b/rust/helpers/device.c
> @@ -8,3 +8,13 @@ int rust_helper_devm_add_action(struct device *dev,
>  {
>  	return devm_add_action(dev, action, data);
>  }
> +
> +void *rust_helper_dev_get_drvdata(const struct device *dev)
> +{
> +	return dev_get_drvdata(dev);
> +}
> +
> +void rust_helper_dev_set_drvdata(struct device *dev, void *data)
> +{
> +	dev_set_drvdata(dev, data);
> +}
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index e9094d8322d5..146eba147d2f 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -6,7 +6,7 @@
>  
>  use crate::{
>      bindings,
> -    types::{ARef, Opaque},
> +    types::{ARef, ForeignOwnable, Opaque},
>  };
>  use core::{fmt, marker::PhantomData, ptr};
>  
> @@ -62,6 +62,47 @@ pub unsafe fn get_device(ptr: *mut bindings::device) -> ARef<Self> {
>      }
>  }
>  
> +impl Device<Internal> {
> +    /// Store a pointer to the bound driver's private data.
> +    pub fn set_drvdata(&self, data: impl ForeignOwnable) {
> +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> +        unsafe { bindings::dev_set_drvdata(self.as_raw(), data.into_foreign().cast()) }
> +    }

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


> +
> +    /// Take ownership of the private data stored in this [`Device`].
> +    ///
> +    /// # Safety
> +    ///
> +    /// - Must only be called once after a preceding call to [`Device::set_drvdata`].
> +    /// - The type `T` must match the type of the `ForeignOwnable` previously stored by
> +    ///   [`Device::set_drvdata`].
> +    pub unsafe fn drvdata_obtain<T: ForeignOwnable>(&self) -> T {
> +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> +        let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };
> +
> +        // SAFETY: By the safety requirements of this function, `ptr` comes from a previous call to
> +        // `into_foreign()`.
> +        unsafe { T::from_foreign(ptr.cast()) }
> +    }
> +
> +    /// Borrow the driver's private data bound to this [`Device`].
> +    ///
> +    /// # Safety
> +    ///
> +    /// - Must only be called after a preceding call to [`Device::set_drvdata`] and before
> +    ///   [`Device::drvdata_obtain`].
> +    /// - The type `T` must match the type of the `ForeignOwnable` previously stored by
> +    ///   [`Device::set_drvdata`].
> +    pub unsafe fn drvdata_borrow<T: ForeignOwnable>(&self) -> T::Borrowed<'_> {
> +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> +        let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };
> +
> +        // SAFETY: By the safety requirements of this function, `ptr` comes from a previous call to
> +        // `into_foreign()`.
> +        unsafe { T::borrow(ptr.cast()) }
> +    }
> +}

Why can't this be part of Core?

thanks,

greg k-h

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

> > +impl Device<Internal> {
> > +    /// Store a pointer to the bound driver's private data.
> > +    pub fn set_drvdata(&self, data: impl ForeignOwnable) {
> > +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> > +        unsafe { bindings::dev_set_drvdata(self.as_raw(), data.into_foreign().cast()) }
> > +    }
> 
> Ah, but a driver's private data in the device is NOT a bus-specific
> thing, it's a driver-specific thing, so your previous patch about
> Internal being there for busses feels odd.

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

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

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

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

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

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

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

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

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

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

> > +
> > +    /// Take ownership of the private data stored in this [`Device`].
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// - Must only be called once after a preceding call to [`Device::set_drvdata`].
> > +    /// - The type `T` must match the type of the `ForeignOwnable` previously stored by
> > +    ///   [`Device::set_drvdata`].
> > +    pub unsafe fn drvdata_obtain<T: ForeignOwnable>(&self) -> T {
> > +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> > +        let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };
> > +
> > +        // SAFETY: By the safety requirements of this function, `ptr` comes from a previous call to
> > +        // `into_foreign()`.
> > +        unsafe { T::from_foreign(ptr.cast()) }
> > +    }
> > +
> > +    /// Borrow the driver's private data bound to this [`Device`].
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// - Must only be called after a preceding call to [`Device::set_drvdata`] and before
> > +    ///   [`Device::drvdata_obtain`].
> > +    /// - The type `T` must match the type of the `ForeignOwnable` previously stored by
> > +    ///   [`Device::set_drvdata`].
> > +    pub unsafe fn drvdata_borrow<T: ForeignOwnable>(&self) -> T::Borrowed<'_> {
> > +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> > +        let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };
> > +
> > +        // SAFETY: By the safety requirements of this function, `ptr` comes from a previous call to
> > +        // `into_foreign()`.
> > +        unsafe { T::borrow(ptr.cast()) }
> > +    }
> > +}
> 
> Why can't this be part of Core?

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

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

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

On Tue, Jul 01, 2025 at 12:58:24PM +0200, Danilo Krummrich wrote:
> On Tue, Jul 01, 2025 at 11:27:54AM +0200, Greg KH wrote:
> > On Sat, Jun 21, 2025 at 09:43:28PM +0200, Danilo Krummrich wrote:
> 
> > > +impl Device<Internal> {
> > > +    /// Store a pointer to the bound driver's private data.
> > > +    pub fn set_drvdata(&self, data: impl ForeignOwnable) {
> > > +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> > > +        unsafe { bindings::dev_set_drvdata(self.as_raw(), data.into_foreign().cast()) }
> > > +    }
> > 
> > Ah, but a driver's private data in the device is NOT a bus-specific
> > thing, it's a driver-specific thing, so your previous patch about
> > Internal being there for busses feels odd.
> 
> It's because we only want to allow the bus abstraction to call
> Device::set_drvdata().
> 
> The reason is the lifecycle of the driver's private data:
> 
> It starts when the driver returns the private data object in probe(). In the bus
> abstraction's probe() function, we're calling set_drvdata().
> 
> At this point the ownership of the object technically goes to the device. And it
> is our responsibility to extract the object from dev->driver_data at some point
> again through drvdata_obtain(). With calling drvdata_obtain() we take back
> ownership of the object.
> 
> Obviously, we do this in the bus abstraction's remove() callback, where we then
> let the object go out of scope, such that it's destructor gets called.
> 
> In contrast, drvdata_borrow() does what its name implies, it only borrows the
> object from dev->driver_data, such that we can provide it for the driver to use.
> 
> In the bus abstraction's remove() callback, drvdata_obtain() must be able to
> proof that the object we extract from dev->driver_data is the exact object that
> we set when calling set_drvdata() from probe().
> 
> If we'd allow the driver to call set_drvdata() itself (which is unnecessary
> anyways), drivers could:
> 
>   1) Call set_drvdata() multiple times, where every previous call would leak the
>      object, since the pointer would be overwritten.
> 
>   2) We'd loose any guarantee about the type we extract from dev->driver_data
>      in the bus abstraction's remove() callback wioth drvdata_obtain().
> 
> > > +
> > > +    /// Take ownership of the private data stored in this [`Device`].
> > > +    ///
> > > +    /// # Safety
> > > +    ///
> > > +    /// - Must only be called once after a preceding call to [`Device::set_drvdata`].
> > > +    /// - The type `T` must match the type of the `ForeignOwnable` previously stored by
> > > +    ///   [`Device::set_drvdata`].
> > > +    pub unsafe fn drvdata_obtain<T: ForeignOwnable>(&self) -> T {
> > > +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> > > +        let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };
> > > +
> > > +        // SAFETY: By the safety requirements of this function, `ptr` comes from a previous call to
> > > +        // `into_foreign()`.
> > > +        unsafe { T::from_foreign(ptr.cast()) }
> > > +    }
> > > +
> > > +    /// Borrow the driver's private data bound to this [`Device`].
> > > +    ///
> > > +    /// # Safety
> > > +    ///
> > > +    /// - Must only be called after a preceding call to [`Device::set_drvdata`] and before
> > > +    ///   [`Device::drvdata_obtain`].
> > > +    /// - The type `T` must match the type of the `ForeignOwnable` previously stored by
> > > +    ///   [`Device::set_drvdata`].
> > > +    pub unsafe fn drvdata_borrow<T: ForeignOwnable>(&self) -> T::Borrowed<'_> {
> > > +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> > > +        let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };
> > > +
> > > +        // SAFETY: By the safety requirements of this function, `ptr` comes from a previous call to
> > > +        // `into_foreign()`.
> > > +        unsafe { T::borrow(ptr.cast()) }
> > > +    }
> > > +}
> > 
> > Why can't this be part of Core?
> 
> Device::drvdata_borrow() itself can indeed be part of Core. It has to remain
> unsafe though, because the type T has to match the type that the driver returned
> from probe().
> 
> Instead, we should provide a reference of the driver's private data in every bus
> callback, such that drivers don't need unsafe code.
> 
> In order to not tempt drivers to use the unsafe method drvdata_borrow()
> directly, I went for hiding it behind the BusInternal device context.

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

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

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

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

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