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

Alice Ryhl
2025-10-29T17:20:47Z

On Wed, Oct 29, 2025 at 06:02:45PM +0100, Danilo Krummrich wrote:
> On Wed Oct 29, 2025 at 4:30 PM CET, Danilo Krummrich wrote:
> > On Wed Oct 29, 2025 at 1:59 PM CET, Alice Ryhl wrote:
> >> Are you going to open that docs PR to the Rust compiler about the size
> >> of TypeID that we talked about? :)
> >
> > Yes, I will -- thanks for the reminder.
> >
> >> 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.
> >
> > Indeed, good catch -- same for the suggestions below.
> >
> >> 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?
>
> Here's the diff, including a missing check in case someone tries to call
> Device::drvdata() from probe().
>
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index 36c6eec0ceab..1a307be953c2 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -17,11 +17,8 @@
>
> pub mod property;
>
> -// 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>());
> -};
> +// 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>());
>
> /// The core representation of a device in the kernel's driver model.
> ///
> @@ -204,7 +201,7 @@ pub unsafe fn as_bound(&self) -> &Device<Bound> {
> }
>
> impl Device<CoreInternal> {
> - fn type_id_store<T: 'static>(&self) {
> + fn set_type_id<T: 'static>(&self) {
> // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> let private = unsafe { (*self.as_raw()).p };
>
> @@ -226,7 +223,7 @@ pub fn set_drvdata<T: 'static>(&self, data: impl PinInit<T, Error>) -> Result {
>
> // 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()) };
> - self.type_id_store::<T>();
> + self.set_type_id::<T>();
>
> Ok(())
> }
> @@ -242,6 +239,9 @@ pub unsafe fn drvdata_obtain<T: 'static>(&self) -> Pin<KBox<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 type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> + unsafe { bindings::dev_set_drvdata(self.as_raw(), core::ptr::null_mut()) };
> +
> // SAFETY:
> // - By the safety requirements of this function, `ptr` comes from a previous call to
> // `into_foreign()`.
> @@ -286,7 +286,7 @@ unsafe fn drvdata_unchecked<T: 'static>(&self) -> Pin<&T> {
> unsafe { Pin::<KBox<T>>::borrow(ptr.cast()) }
> }
>
> - fn type_id_match<T: 'static>(&self) -> Result {
> + fn match_type_id<T: 'static>(&self) -> Result {
> // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> let private = unsafe { (*self.as_raw()).p };
>
> @@ -311,11 +311,16 @@ fn type_id_match<T: 'static>(&self) -> Result {
> /// Returns a pinned reference to the driver's private data or [`EINVAL`] if it doesn't match
> /// the asserted type `T`.
> pub fn drvdata<T: 'static>(&self) -> Result<Pin<&T>> {
> - self.type_id_match::<T>()?;
> + // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> + if unsafe { bindings::dev_get_drvdata(self.as_raw()) }.is_null() {
> + return Err(ENOENT);
> + }
> +
> + self.match_type_id::<T>()?;
>
> // SAFETY:
> - // - The `Bound` device context guarantees that this is only ever call after a call
> - // to `set_drvdata()` and before `drvdata_obtain()`.
> + // - The above check of `dev_get_drvdata()` guarantees that we are called after
> + // `set_drvdata()` and before `drvdata_obtain()`.
> // - We've just checked that the type of the driver's private data is in fact `T`.
> Ok(unsafe { self.drvdata_unchecked() })
> }
>

this looks ok to me.