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

[Danilo Krummrich]

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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