public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
From: "Gary Guo" <gary@garyguo.net>
To: "Danilo Krummrich" <dakr@kernel.org>,
	<gregkh@linuxfoundation.org>, <rafael@kernel.org>,
	<ojeda@kernel.org>, <boqun@kernel.org>, <gary@garyguo.net>,
	<bjorn3_gh@protonmail.com>, <lossin@kernel.org>,
	<a.hindborg@kernel.org>, <aliceryhl@google.com>,
	<tmgross@umich.edu>, <acourbot@nvidia.com>,
	<ecourtney@nvidia.com>, <m.wilczynski@samsung.com>,
	<david.m.ertman@intel.com>, <ira.weiny@intel.com>,
	<leon@kernel.org>, <daniel.almeida@collabora.com>,
	<bhelgaas@google.com>, <kwilczynski@kernel.org>
Cc: <driver-core@lists.linux.dev>, <linux-kernel@vger.kernel.org>,
	<nova-gpu@lists.linux.dev>, <dri-devel@lists.freedesktop.org>,
	<linux-pwm@vger.kernel.org>, <rust-for-linux@vger.kernel.org>
Subject: Re: [PATCH v2 3/7] rust: auxiliary: add registration_data_with() for ForLt types
Date: Wed, 03 Jun 2026 13:05:45 +0100	[thread overview]
Message-ID: <DIZEFXX93XUI.38M3E0EM9VN89@garyguo.net> (raw)
In-Reply-To: <20260603011020.2073650-4-dakr@kernel.org>

On Wed Jun 3, 2026 at 2:10 AM BST, Danilo Krummrich wrote:
> Add registration_data_with() taking a for<'a> closure that receives
> Pin<&'a F::Of<'a>>, which works with any ForLt type. Taking a for<'a>
> closure rather than returning a direct reference prevents callers from
> choosing a concrete lifetime for the data, which is required for
> soundness with non-covariant ForLt types.
>
> Extract the common null-check, TypeId-check and KBox-borrow logic into a
> private registration_data_pinned() helper shared by both
> registration_data_with() and the existing registration_data().
>
> Relax Registration's bound from CovariantForLt to ForLt so that
> non-covariant types can be registered.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/auxiliary.rs | 89 ++++++++++++++++++++++++++++++----------
>  1 file changed, 68 insertions(+), 21 deletions(-)
>
> diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs
> index 40a0af74a8e5..81549a3e347e 100644
> --- a/rust/kernel/auxiliary.rs
> +++ b/rust/kernel/auxiliary.rs
> @@ -21,6 +21,7 @@
>      prelude::*,
>      types::{
>          CovariantForLt,
> +        ForLt,
>          ForeignOwnable,
>          Opaque, //
>      },
> @@ -270,18 +271,15 @@ pub fn parent(&self) -> &device::Device<device::Bound> {
>          unsafe { parent.as_bound() }
>      }
>  
> -    /// Returns a pinned reference to the registration data set by the registering (parent) driver.
> +    /// Returns the stored registration data as a pinned `'static` reference.
>      ///
> -    /// `F` is the [`CovariantForLt`](trait@CovariantForLt) encoding of the data type. The returned
> -    /// reference has its lifetime shortened from `'static` to `&self`'s borrow lifetime via
> -    /// [`CovariantForLt::cast_ref`].
> +    /// Performs null and [`TypeId`] checks, then borrows the stored [`KBox`].
>      ///
> -    /// Returns [`EINVAL`] if `F` does not match the type used by the parent driver when calling
> -    /// [`Registration::new()`].
> +    /// # Safety
>      ///
> -    /// Returns [`ENOENT`] if no registration data has been set, e.g. when the device was
> -    /// registered by a C driver.
> -    pub fn registration_data<F: CovariantForLt + 'static>(&self) -> Result<Pin<&F::Of<'_>>> {
> +    /// The returned `'static` lifetime was transmuted from the device's bound lifetime during
> +    /// registration. Callers must shorten it before exposing it.
> +    unsafe fn registration_data_pinned<F: ForLt + 'static>(&self) -> Result<Pin<&F::Of<'static>>> {
>          // SAFETY: By the type invariant, `self.as_raw()` is a valid `struct auxiliary_device`.
>          let ptr = unsafe { (*self.as_raw()).registration_data_rust };
>          if ptr.is_null() {
> @@ -306,10 +304,58 @@ pub fn registration_data<F: CovariantForLt + 'static>(&self) -> Result<Pin<&F::O
>          let wrapper = unsafe { Pin::<KBox<RegistrationData<F::Of<'static>>>>::borrow(ptr) };
>  
>          // SAFETY: `data` is a structurally pinned field of `RegistrationData`.
> -        let pinned: Pin<&F::Of<'_>> = unsafe { wrapper.map_unchecked(|w| &w.data) };
> +        Ok(unsafe { wrapper.map_unchecked(|w| &w.data) })
> +    }
> +
> +    /// Access the registration data set by the registering (parent) driver through a closure.
> +    ///
> +    /// `F` is the [`ForLt`](trait@ForLt) encoding of the data type. The closure receives a pinned
> +    /// reference to the registration data.
> +    ///
> +    /// For covariant types that implement [`trait@CovariantForLt`], prefer
> +    /// [`registration_data`](Self::registration_data) which returns a direct reference.
> +    ///
> +    /// Returns [`EINVAL`] if `F` does not match the type used by the parent driver when calling
> +    /// [`Registration::new()`].
> +    ///
> +    /// Returns [`ENOENT`] if no registration data has been set, e.g. when the device was
> +    /// registered by a C driver.
> +    pub fn registration_data_with<F: ForLt + 'static, R>(
> +        &self,
> +        f: impl for<'a> FnOnce(Pin<&'a F::Of<'a>>) -> R,
> +    ) -> Result<R> {
> +        // SAFETY: The HRTB closure prevents the caller from smuggling in references with a
> +        // concrete short lifetime, making the round-trip from `'static` sound regardless of
> +        // variance.
> +        let pinned = unsafe { self.registration_data_pinned::<F>()? };
> +
> +        // SAFETY: See above; the closure's HRTB makes the round-trip sound.
> +        let short = unsafe { F::cast_ref_unchecked(pinned.get_ref()) };

This shouldn't be necessary, you can just directly call `f`.

Best,
Gary

> +
> +        // SAFETY: The data was pinned before the lifetime was shortened; pinning is
> +        // orthogonal to lifetimes.
> +        Ok(f(unsafe { Pin::new_unchecked(short) }))
> +    }
>  
> -        // SAFETY: The data was pinned when stored; `cast_ref` only shortens
> -        // the lifetime, so the pinning guarantee is preserved.
> +    /// Returns a pinned reference to the registration data set by the registering (parent) driver.
> +    ///
> +    /// This method is only available when `F` implements [`trait@CovariantForLt`], which guarantees
> +    /// safe lifetime shortening via [`CovariantForLt::cast_ref`].
> +    ///
> +    /// For non-covariant types, use the closure-based [`Self::registration_data_with`].
> +    ///
> +    /// Returns [`EINVAL`] if `F` does not match the type used by the parent driver when calling
> +    /// [`Registration::new()`].
> +    ///
> +    /// Returns [`ENOENT`] if no registration data has been set, e.g. when the device was
> +    /// registered by a C driver.
> +    pub fn registration_data<F: CovariantForLt + 'static>(&self) -> Result<Pin<&F::Of<'_>>> {
> +        // SAFETY: CovariantForLt guarantees covariance, so cast_ref safely shortens the
> +        // `'static` lifetime.
> +        let pinned = unsafe { self.registration_data_pinned::<F>()? };
> +
> +        // SAFETY: The data was pinned before the lifetime was shortened; pinning is orthogonal
> +        // to lifetimes.
>          Ok(unsafe { Pin::new_unchecked(F::cast_ref(pinned.get_ref())) })
>      }
>  }
> @@ -399,22 +445,23 @@ struct RegistrationData<T> {
>  /// This type represents the registration of a [`struct auxiliary_device`]. When its parent device
>  /// is unbound, the corresponding auxiliary device will be unregistered from the system.
>  ///
> -/// The type parameter `F` is a [`CovariantForLt`](trait@CovariantForLt) encoding of the
> -/// registration data type. For non-lifetime-parameterized types, use
> -/// [`CovariantForLt!(T)`](macro@CovariantForLt).
> -/// The data can be accessed by the auxiliary driver through [`Device::registration_data()`].
> +/// The type parameter `F` is a [`ForLt`](trait@ForLt) encoding of the registration
> +/// data type. For non-lifetime-parameterized types, use [`ForLt!(T)`](macro@ForLt).
> +///
> +/// The data can be accessed by the auxiliary driver through [`Device::registration_data()`] and
> +/// [`Device::registration_data_with()`].
>  ///
>  /// # Invariants
>  ///
>  /// `self.adev` always holds a valid pointer to an initialized and registered
>  /// [`struct auxiliary_device`] whose `registration_data_rust` field points to a
>  /// valid `Pin<KBox<RegistrationData<F::Of<'static>>>>`.
> -pub struct Registration<'a, F: CovariantForLt + 'static> {
> +pub struct Registration<'a, F: ForLt + 'static> {
>      adev: NonNull<bindings::auxiliary_device>,
>      _phantom: PhantomData<F::Of<'a>>,
>  }
>  
> -impl<'a, F: CovariantForLt> Registration<'a, F>
> +impl<'a, F: ForLt> Registration<'a, F>
>  where
>      for<'b> F::Of<'b>: Send + Sync,
>  {
> @@ -526,7 +573,7 @@ pub fn new<E>(
>      }
>  }
>  
> -impl<F: CovariantForLt> Drop for Registration<'_, F> {
> +impl<F: ForLt> Drop for Registration<'_, F> {
>      fn drop(&mut self) {
>          // SAFETY: By the type invariant of `Self`, `self.adev.as_ptr()` is a valid registered
>          // `struct auxiliary_device`.
> @@ -548,7 +595,7 @@ fn drop(&mut self) {
>  }
>  
>  // SAFETY: A `Registration` of a `struct auxiliary_device` can be released from any thread.
> -unsafe impl<F: CovariantForLt> Send for Registration<'_, F> where for<'a> F::Of<'a>: Send {}
> +unsafe impl<F: ForLt> Send for Registration<'_, F> where for<'a> F::Of<'a>: Send {}
>  
>  // SAFETY: `Registration` does not expose any methods or fields that need synchronization.
> -unsafe impl<F: CovariantForLt> Sync for Registration<'_, F> where for<'a> F::Of<'a>: Send {}
> +unsafe impl<F: ForLt> Sync for Registration<'_, F> where for<'a> F::Of<'a>: Send {}



  reply	other threads:[~2026-06-03 12:05 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-06-03  1:10 [PATCH v2 0/7] ForLt/CovariantForLt split, auxiliary closure API and DevresLt Danilo Krummrich
2026-06-03  1:10 ` [PATCH v2 1/7] rust: types: rename ForLt to CovariantForLt Danilo Krummrich
2026-06-03 11:59   ` Gary Guo
2026-06-04  2:08   ` Claude review: " Claude Code Review Bot
2026-06-03  1:10 ` [PATCH v2 2/7] rust: types: introduce ForLt base trait for CovariantForLt Danilo Krummrich
2026-06-03 12:04   ` Gary Guo
2026-06-04  2:08   ` Claude review: " Claude Code Review Bot
2026-06-03  1:10 ` [PATCH v2 3/7] rust: auxiliary: add registration_data_with() for ForLt types Danilo Krummrich
2026-06-03 12:05   ` Gary Guo [this message]
2026-06-04  2:08   ` Claude review: " Claude Code Review Bot
2026-06-03  1:10 ` [PATCH v2 4/7] rust: auxiliary: sample: demonstrate ForLt with invariant Mutex type Danilo Krummrich
2026-06-04  2:08   ` Claude review: " Claude Code Review Bot
2026-06-03  1:10 ` [PATCH v2 5/7] rust: devres: add DevresLt for ForLt-aware device resource access Danilo Krummrich
2026-06-04  2:08   ` Claude review: " Claude Code Review Bot
2026-06-03  1:10 ` [PATCH v2 6/7] rust: pci: return DevresLt from Bar::into_devres() Danilo Krummrich
2026-06-04  2:08   ` Claude review: " Claude Code Review Bot
2026-06-03  1:10 ` [PATCH v2 7/7] rust: io: mem: return DevresLt from IoMem/ExclusiveIoMem::into_devres() Danilo Krummrich
2026-06-04  2:08   ` Claude review: " Claude Code Review Bot
2026-06-04  2:08 ` Claude review: ForLt/CovariantForLt split, auxiliary closure API and DevresLt Claude Code Review Bot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DIZEFXX93XUI.38M3E0EM9VN89@garyguo.net \
    --to=gary@garyguo.net \
    --cc=a.hindborg@kernel.org \
    --cc=acourbot@nvidia.com \
    --cc=aliceryhl@google.com \
    --cc=bhelgaas@google.com \
    --cc=bjorn3_gh@protonmail.com \
    --cc=boqun@kernel.org \
    --cc=dakr@kernel.org \
    --cc=daniel.almeida@collabora.com \
    --cc=david.m.ertman@intel.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=driver-core@lists.linux.dev \
    --cc=ecourtney@nvidia.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=ira.weiny@intel.com \
    --cc=kwilczynski@kernel.org \
    --cc=leon@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=lossin@kernel.org \
    --cc=m.wilczynski@samsung.com \
    --cc=nova-gpu@lists.linux.dev \
    --cc=ojeda@kernel.org \
    --cc=rafael@kernel.org \
    --cc=rust-for-linux@vger.kernel.org \
    --cc=tmgross@umich.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox