Re: [PATCH 2/8] rust: device: introduce Device::drvdata()

Alice Ryhl
2025-10-29T12:59:43Z

On Tue, Oct 21, 2025 at 12:34:24AM +0200, Danilo Krummrich wrote:
> 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.
>
> 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).
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Are you going to open that docs PR to the Rust compiler about the size
of TypeID that we talked about? :)

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

> +// Compile-time checks.
> +const _: () = {
> + // Assert that we can `read()` / `write()` a `TypeId` instance from / into `struct driver_type`.
> + static_assert!(core::mem::size_of::<bindings::driver_type>() == core::mem::size_of::<TypeId>());
> +};

You don't need the "const _: ()" part. See the definition of
static_assert! to see why.

Also, I would not require equality. The Rust team did not think that it
would ever increase in size, but it may decrease.

> /// The core representation of a device in the kernel's driver model.
> ///
> /// This structure represents the Rust abstraction for a C `struct device`. A [`Device`] can either
> @@ -198,12 +204,29 @@ pub unsafe fn as_bound(&self) -> &Device<Bound> {
> }
>
> impl Device<CoreInternal> {
> + fn type_id_store<T: 'static>(&self) {

This name isn't great. How about "set_type_id()" instead?

Alice