Message-ID: <aQIQl8lMhztucZhK@google.com>
Date: Wed, 29 Oct 2025 13:03:19 +0000
From: Alice Ryhl <aliceryhl@...gle.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, bhelgaas@...gle.com, 
	kwilczynski@...nel.org, david.m.ertman@...el.com, ira.weiny@...el.com, 
	leon@...nel.org, acourbot@...dia.com, ojeda@...nel.org, alex.gaynor@...il.com, 
	boqun.feng@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com, 
	lossin@...nel.org, a.hindborg@...nel.org, tmgross@...ch.edu, 
	pcolberg@...hat.com, rust-for-linux@...r.kernel.org, 
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/8] Device::drvdata() and driver/driver interaction (auxiliary)

On Tue, Oct 21, 2025 at 12:34:22AM +0200, Danilo Krummrich wrote:
> tl;dr:
> 
> Implement a safe Device<Bound>::drvdata() accessor (used for driver to
> driver interactions) based on the auxiliary bus.
> 
> This provides a way to derive a driver's device private data when
> serving as a parent in a driver hierarchy, such as a driver utilizing
> the auxiliary bus.
> 
> Please have a look at patch 8 ("samples: rust: auxiliary: illustrate
> driver interaction") to see how it turns out.
> 
> --
> 
> Full cover letter:
> 
> In C dev_get_drvdata() has specific requirements under which it is valid
> to access the returned pointer. That is, drivers have to ensure that
> 
>   (1) for the duration the returned pointer is accessed the driver is
>       bound and remains to be bound to the corresponding device,
> 
>   (2) the returned void * is treated according to the driver's private
>       data type, i.e. according to what has been passed to
>       dev_set_drvdata().
> 
> In Rust, (1) can be ensured by simply requiring the Bound device
> context, i.e. provide the drvdata() method for Device<Bound> only.
> 
> For (2) we would usually make the device type generic over the driver
> type, e.g. Device<T: Driver>, where <T as Driver>::Data is the type of
> the driver's private data.
> 
> However, a device does not have a driver type known at compile time and
> may be bound to multiple drivers throughout its lifetime.
> 
> Hence, in order to be able to provide a safe accessor for the driver's
> device private data, we have to do the type check on runtime.
> 
> This is achieved by letting a driver assert the expected type, which is
> then compared to a type hash stored in struct device_private when
> dev_set_drvdata() is called [2].
> 
> Example:
> 
>         // `dev` is a `&Device<Bound>`.
>         let data = dev.drvdata::<SampleDriver>()?;
> 
> There are two aspects to note:
> 
>   (1) Technically, the same check could be achieved by comparing the
>       struct device_driver pointer of struct device with the struct
>       device_driver pointer of the driver struct (e.g. struct
>       pci_driver).
> 
>       However, this would - in addition the pointer comparison - require
>       to tie back the private driver data type to the struct
>       device_driver pointer of the driver struct to prove correctness.
> 
>       Besides that, accessing the driver struct (stored in the module
>       structure) isn't trivial and would result into horrible code and
>       API ergonomics.
> 
>   (2) Having a direct accessor to the driver's private data is not
>       commonly required (at least in Rust): Bus callback methods already
>       provide access to the driver's device private data through a &self
>       argument, while other driver entry points such as IRQs,
>       workqueues, timers, IOCTLs, etc. have their own private data with
>       separate ownership and lifetime.
> 
>       In other words, a driver's device private data is only relevant
>       for driver model contexts (such a file private is only relevant
>       for file contexts).
> 
> Having that said, the motivation for accessing the driver's device
> private data with Device<Bound>::drvdata() are interactions between
> drivers. For instance, when an auxiliary driver calls back into its
> parent, the parent has to be capable to derive its private data from the
> corresponding device (i.e. the parent of the auxiliary device).
> 
> Therefore this patch series also contains the corresponding patches for
> the auxiliary bus abstraction, i.e. guarantee that the auxiliary
> device's parent is guaranteed to be bound when the auxiliary device
> itself is guaranteed to be bound, plus the corresponding
> Device<Bound>::parent() method.
> 
> Finally, illustrate how things turn out by updating the auxiliary sample
> driver.
> 
> Similarly, the same thing can be done for PCI virtual function drivers
> calling back into the corresponding physical function driver or MFD.
> 
> The former (PCI PF/VF interaction) will be addressed by a separate patch
> series. Both, auxiliary and PCI PF/VF is required by the Nova project.
> 
> A branch containing the series can be found in [1].
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=drvdata
> [2] Type hash (TypeId) stored in struct device_private:
> 
>         The Rust type stored in struct device_private could be replaced
>         by a dedicated (and transparent) private pointer (e.g.
>         struct device_private::rust).
> 
>         While I'm not overly concerned about the extra allocation (not a
>         hot path at all), I still wanted to try to store it directly in
>         struct device_private, see how it turns out and gather opinions.
> 
>         Additionally, I don't expect any additional Rust specific
>         private data to be required. But even if, changing things up to
>         use a separate transparent allocation in the future is trivial.
> 
> Danilo Krummrich (8):
>   rust: device: narrow the generic of drvdata_obtain()
>   rust: device: introduce Device::drvdata()
>   rust: auxiliary: consider auxiliary devices always have a parent
>   rust: auxiliary: unregister on parent device unbind
>   rust: auxiliary: move parent() to impl Device
>   rust: auxiliary: implement parent() for Device<Bound>
>   samples: rust: auxiliary: misc cleanup of ParentDriver::connect()
>   samples: rust: auxiliary: illustrate driver interaction
> 
>  drivers/base/base.h                   |  16 ++++
>  drivers/gpu/drm/nova/file.rs          |   2 +-
>  drivers/gpu/nova-core/driver.rs       |   8 +-
>  rust/bindings/bindings_helper.h       |   6 ++
>  rust/kernel/auxiliary.rs              | 108 ++++++++++++++++----------
>  rust/kernel/device.rs                 |  83 ++++++++++++++++++--
>  rust/kernel/pci.rs                    |   2 +-
>  rust/kernel/platform.rs               |   2 +-
>  rust/kernel/usb.rs                    |   4 +-
>  samples/rust/rust_driver_auxiliary.rs |  44 +++++++----
>  10 files changed, 207 insertions(+), 68 deletions(-)

It looks like there are some patches that add code that doesn't pass
rustfmt, which are then fixed in follow-up commits. You might want to do
a pass of rustfmt after each commit.

Reviewed-by: Alice Ryhl <aliceryhl@...gle.com>

Powered by blists - more mailing lists