Message-ID: <aGPesxbxR-ob_Hqr@pollux>
Date: Tue, 1 Jul 2025 15:12:19 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: rafael@...nel.org, ojeda@...nel.org, alex.gaynor@...il.com,
	boqun.feng@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com,
	benno.lossin@...ton.me, a.hindborg@...nel.org, aliceryhl@...gle.com,
	tmgross@...ch.edu, david.m.ertman@...el.com, ira.weiny@...el.com,
	leon@...nel.org, kwilczynski@...nel.org, bhelgaas@...gle.com,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org
Subject: Re: [PATCH 2/8] rust: device: add drvdata accessors

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.

Powered by blists - more mailing lists