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

[Alice Ryhl <aliceryhl@google.com>]

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