linux-kernel - Re: [PATCH 4/8] rust: pci: use generic device drvdata accessors

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2025070142-scrambler-ramp-cd7e@gregkh>
Date: Tue, 1 Jul 2025 11:30:11 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Danilo Krummrich <dakr@...nel.org>
Cc: rafael@...nel.org, ojeda@...nel.org, alex.gaynor@...il.com,
	boqun.feng@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com,
	benno.lossin@...ton.me, a.hindborg@...nel.org, aliceryhl@...gle.com,
	tmgross@...ch.edu, david.m.ertman@...el.com, ira.weiny@...el.com,
	leon@...nel.org, kwilczynski@...nel.org, bhelgaas@...gle.com,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org
Subject: Re: [PATCH 4/8] rust: pci: use generic device drvdata accessors

On Sat, Jun 21, 2025 at 09:43:30PM +0200, Danilo Krummrich wrote:
> Take advantage of the generic drvdata accessors of the generic Device
> type.
> 
> While at it, use from_result() instead of match.
> 
> Signed-off-by: Danilo Krummrich <dakr@...nel.org>
> ---
>  rust/helpers/pci.c | 10 ----------
>  rust/kernel/pci.rs | 31 ++++++++++++++-----------------
>  2 files changed, 14 insertions(+), 27 deletions(-)
> 
> diff --git a/rust/helpers/pci.c b/rust/helpers/pci.c
> index cd0e6bf2cc4d..ef9cb38c81a6 100644
> --- a/rust/helpers/pci.c
> +++ b/rust/helpers/pci.c
> @@ -2,16 +2,6 @@
>  
>  #include <linux/pci.h>
>  
> -void rust_helper_pci_set_drvdata(struct pci_dev *pdev, void *data)
> -{
> -	pci_set_drvdata(pdev, data);
> -}
> -
> -void *rust_helper_pci_get_drvdata(struct pci_dev *pdev)
> -{
> -	return pci_get_drvdata(pdev);
> -}
> -
>  resource_size_t rust_helper_pci_resource_len(struct pci_dev *pdev, int bar)
>  {
>  	return pci_resource_len(pdev, bar);
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 8435f8132e38..064e74a90904 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -10,11 +10,11 @@
>      device_id::RawDeviceId,
>      devres::Devres,
>      driver,
> -    error::{to_result, Result},
> +    error::{from_result, to_result, Result},
>      io::Io,
>      io::IoRaw,
>      str::CStr,
> -    types::{ARef, ForeignOwnable, Opaque},
> +    types::{ARef, Opaque},
>      ThisModule,
>  };
>  use core::{
> @@ -66,35 +66,32 @@ extern "C" fn probe_callback(
>          // `struct pci_dev`.
>          //
>          // INVARIANT: `pdev` is valid for the duration of `probe_callback()`.
> -        let pdev = unsafe { &*pdev.cast::<Device<device::Core>>() };
> +        let pdev = unsafe { &*pdev.cast::<Device<device::Internal>>() };
>  
>          // SAFETY: `DeviceId` is a `#[repr(transparent)` wrapper of `struct pci_device_id` and
>          // does not add additional invariants, so it's safe to transmute.
>          let id = unsafe { &*id.cast::<DeviceId>() };
>          let info = T::ID_TABLE.info(id.index());
>  
> -        match T::probe(pdev, info) {
> -            Ok(data) => {
> -                // Let the `struct pci_dev` own a reference of the driver's private data.
> -                // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
> -                // `struct pci_dev`.
> -                unsafe { bindings::pci_set_drvdata(pdev.as_raw(), data.into_foreign() as _) };
> -            }
> -            Err(err) => return Error::to_errno(err),
> -        }
> +        from_result(|| {
> +            let data = T::probe(pdev, info)?;
>  
> -        0
> +            pdev.as_ref().set_drvdata(data);
> +            Ok(0)
> +        })
>      }
>  
>      extern "C" fn remove_callback(pdev: *mut bindings::pci_dev) {
>          // SAFETY: The PCI bus only ever calls the remove callback with a valid pointer to a
>          // `struct pci_dev`.
> -        let ptr = unsafe { bindings::pci_get_drvdata(pdev) }.cast();
> +        //
> +        // INVARIANT: `pdev` is valid for the duration of `remove_callback()`.
> +        let pdev = unsafe { &*pdev.cast::<Device<device::Internal>>() };
>  
>          // SAFETY: `remove_callback` is only ever called after a successful call to
> -        // `probe_callback`, hence it's guaranteed that `ptr` points to a valid and initialized
> -        // `KBox<T>` pointer created through `KBox::into_foreign`.
> -        let _ = unsafe { KBox::<T>::from_foreign(ptr) };
> +        // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
> +        // and stored a `Pin<KBox<T>>`.
> +        let _ = unsafe { pdev.as_ref().drvdata_obtain::<Pin<KBox<T>>>() };
>      }
>  }
>  
> -- 
> 2.49.0
> 
> 

Overall, I like this, same for the other bus types.  But again, can't it
be part of Core?

thanks,

greg k-h