Message-ID: <aQJM7XPZ-0wtDDCX@google.com>
Date: Wed, 29 Oct 2025 17:20:45 +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 2/8] rust: device: introduce Device::drvdata()

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@...gle.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.

Powered by blists - more mailing lists