From nobody Thu Dec 18 01:50:57 2025 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 97953231836; Fri, 30 May 2025 14:25:11 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1748615111; cv=none; b=CBTlu5hXEBTs1Eqc/fLvhAHrgZwuFy0cht7dGfANwyPGZj6+sUHieSOilsdskZL0lTUMHR4i4mMeO89m+Hw9TBVF3Lk5dQgvD73lu6C+aOS57DFjA3dGOmqoSQjXkA31LwNk+t1/0bW+k9MY+Ebk9XBz62nqrHyfNLsQp2hHGrk= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1748615111; c=relaxed/simple; bh=JqCE9KxtLU73dWPfVR6Y3zqQaSqcJOaV5q9/grUl9Cc=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=g9OLBM0pT+4NoNOC7GPqGWZyvPDUPw5UHAXTKKYJ9K9osKDdotr0Eru2CKpl9Q76KpwTcce3GqOnP54ejSGuus4PqtmaQPewatkpmsstemOnR/cgKLRMcadODe2aj2iBa3Z9yLN1bu1JwNcdin/qfgc6BZD3WbYOY69YsvBgs1A= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=mTbTlNfX; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="mTbTlNfX" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 02988C4CEE9; Fri, 30 May 2025 14:25:07 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1748615111; bh=JqCE9KxtLU73dWPfVR6Y3zqQaSqcJOaV5q9/grUl9Cc=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=mTbTlNfXlMOudoEkl9Ie9Gqjs1KVXKWLSokeAQTbNn3wrGpkFydeck/+eQZaePcKp i9ZwYaP1CCYgDgmldmzqZ4pgdx3Jhm49tVFYD8Rpq5b9kSepxVo1yLMbQwOHvLFk3J vNyXiDJ86p5xiUSeVYncVLKoZLbvHZf5N2ukgyV8fH7vJQ2bVWoiTNCBEKws1SWYn9 BjWDQrgWGfF1X9uJ1NF4CFuhsfZ7BHi5eKJNFUrMgkBat0PKILzJS5GoKSj6wZr02v NOKU0bsKB1ufFTN2EbzsCWUjyNEvsXd6YMXttTo44fZjB7idE+ktDl9v6aEVgEt8Y4 tv0jxT7wb6s/g== From: Danilo Krummrich To: gregkh@linuxfoundation.org, rafael@kernel.org, ojeda@kernel.org, alex.gaynor@gmail.com, boqun.feng@gmail.com, gary@garyguo.net, bjorn3_gh@protonmail.com, benno.lossin@proton.me, a.hindborg@kernel.org, aliceryhl@google.com, tmgross@umich.edu, chrisi.schrefl@gmail.com Cc: rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org, Danilo Krummrich Subject: [PATCH 5/7] rust: miscdevice: properly support device drivers Date: Fri, 30 May 2025 16:24:18 +0200 Message-ID: <20250530142447.166524-6-dakr@kernel.org> X-Mailer: git-send-email 2.49.0 In-Reply-To: <20250530142447.166524-1-dakr@kernel.org> References: <20250530142447.166524-1-dakr@kernel.org> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="utf-8" Currently, the design of MiscDeviceRegistration is focused on creating and registering a misc device directly from a module and hence does not support using misc device from a device driver. However, it is important for the design of the misc device abstraction to take the driver model into account. Hence, consider the use-case of using misc device from a device driver; let's go through the design motivation bottom-up: Ideally, we want to be able to access (bus) device resources (such as I/O memory) from misc device callbacks (such as open() or ioctl()) without additional overhead, i.e. without having to go through Devres::try_access(), which implies an atomic check and an RCU read side critical section. Instead, we want to be able to use Devres::access(), which does not have any overhead, which requires a &Device to prove that we can directly access the device resource. Given that all misc device callbacks are synchronized against misc_deregister(), we can prove that the misc device's parent device is bound iff we guarantee that the misc device's registration won't out-live the parent device's unbind. This can easily be proven by using devres for the misc device's registration object itself. Since this is only applicable for the device driver use-case, abstract the actual registration instance with a Rust enum, which is either a "raw" registration or a "managed" registration. In order to avoid any penalties from a managed registration, structurally separate the registration's private data from the "raw" misc device registration (which either stays "raw" or becomes "managed") depending on whether a parent device is supplied. The advantage of this solution is that it is entirely transparent to the user -- no separate structures or functions for whether the abstraction is used directly from a module or from a device driver; instead MiscDeviceRegistration::register() gets an optional parent argument. Signed-off-by: Danilo Krummrich --- rust/kernel/miscdevice.rs | 178 ++++++++++++++++++++++++------- samples/rust/rust_misc_device.rs | 9 +- 2 files changed, 143 insertions(+), 44 deletions(-) diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs index 1b5ec13868e2..6801fe72a8a6 100644 --- a/rust/kernel/miscdevice.rs +++ b/rust/kernel/miscdevice.rs @@ -10,16 +10,17 @@ =20 use crate::{ bindings, container_of, - device::Device, + device::{Bound, Device}, + devres::Devres, error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR}, ffi::{c_int, c_long, c_uint, c_ulong}, fs::File, prelude::*, seq_file::SeqFile, str::CStr, - types::{ForeignOwnable, Opaque}, + types::{ARef, ForeignOwnable, Opaque}, }; -use core::{marker::PhantomData, mem::MaybeUninit, pin::Pin}; +use core::{marker::PhantomData, mem::MaybeUninit, pin::Pin, ptr::NonNull}; =20 /// Options for creating a misc device. #[derive(Copy, Clone)] @@ -40,44 +41,43 @@ pub const fn into_raw(self) -> bindings:= :miscdevice { } } =20 -/// A registration of a miscdevice. -/// /// # Invariants /// -/// `inner` is a registered misc device. +/// - `inner` is a registered misc device, +/// - `data` is valid for the entire lifetime of `Self`. #[repr(C)] #[pin_data(PinnedDrop)] -pub struct MiscDeviceRegistration { +struct RawDeviceRegistration { #[pin] inner: Opaque, - #[pin] - data: Opaque, + data: NonNull, _t: PhantomData, } =20 -// SAFETY: -// - It is allowed to call `misc_deregister` on a different thread from wh= ere you called -// `misc_register`. -// - Only implements `Send` if `MiscDevice::RegistrationData` is also `Sen= d`. -unsafe impl Send for MiscDeviceRegistration where T::Reg= istrationData: Send {} - -// SAFETY: -// - All `&self` methods on this type are written to ensure that it is saf= e to call them in -// parallel. -// - `MiscDevice::RegistrationData` is always `Sync`. -unsafe impl Sync for MiscDeviceRegistration {} - -impl MiscDeviceRegistration { - /// Register a misc device. - pub fn register( +impl RawDeviceRegistration { + fn new<'a>( opts: MiscDeviceOptions, - data: impl PinInit, - ) -> impl PinInit { + parent: Option<&'a Device>, + data: &'a T::RegistrationData, + ) -> impl PinInit + 'a + where + T: 'a, + { try_pin_init!(Self { - data <- Opaque::pin_init(data), + // INVARIANT: `Self` is always embedded in a `MiscDeviceRegist= ration`, hence `data` + // is guaranteed to be valid for the entire lifetime of `Self`. + data: NonNull::from(data), inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscd= evice| { + let mut value =3D opts.into_raw::(); + + if let Some(parent) =3D parent { + // The device core code will take care to take a refer= ence of `parent` in + // `device_add()` called by `misc_register()`. + value.parent =3D parent.as_raw(); + } + // SAFETY: The initializer can write to the provided `slot= `. - unsafe { slot.write(opts.into_raw::()) }; + unsafe { slot.write(value) }; =20 // SAFETY: // * We just wrote the misc device options to the slot. Th= e miscdevice will @@ -94,12 +94,12 @@ pub fn register( } =20 /// Returns a raw pointer to the misc device. - pub fn as_raw(&self) -> *mut bindings::miscdevice { + fn as_raw(&self) -> *mut bindings::miscdevice { self.inner.get() } =20 /// Access the `this_device` field. - pub fn device(&self) -> &Device { + fn device(&self) -> &Device { // SAFETY: This can only be called after a successful register(), = which always // initialises `this_device` with a valid device. Furthermore, the= signature of this // function tells the borrow-checker that the `&Device` reference = must not outlive the @@ -108,6 +108,108 @@ pub fn device(&self) -> &Device { unsafe { Device::as_ref((*self.as_raw()).this_device) } } =20 + fn data(&self) -> &T::RegistrationData { + // SAFETY: The type invariant guarantees that `data` is valid for = the entire lifetime of + // `Self`. + unsafe { self.data.as_ref() } + } +} + +#[pinned_drop] +impl PinnedDrop for RawDeviceRegistration { + fn drop(self: Pin<&mut Self>) { + // SAFETY: We know that the device is registered by the type invar= iants. + unsafe { bindings::misc_deregister(self.inner.get()) }; + } +} + +#[expect(dead_code)] +enum DeviceRegistrationInner { + Raw(Pin>>), + Managed(Devres>), +} + +/// A registration of a miscdevice. +#[pin_data(PinnedDrop)] +pub struct MiscDeviceRegistration { + inner: DeviceRegistrationInner, + #[pin] + data: Opaque, + this_device: ARef, + _t: PhantomData, +} + +// SAFETY: +// - It is allowed to call `misc_deregister` on a different thread from wh= ere you called +// `misc_register`. +// - Only implements `Send` if `MiscDevice::RegistrationData` is also `Sen= d`. +unsafe impl Send for MiscDeviceRegistration where T::Reg= istrationData: Send {} + +// SAFETY: +// - All `&self` methods on this type are written to ensure that it is saf= e to call them in +// parallel. +// - `MiscDevice::RegistrationData` is always `Sync`. +unsafe impl Sync for MiscDeviceRegistration {} + +impl MiscDeviceRegistration { + /// Register a misc device. + pub fn register<'a>( + opts: MiscDeviceOptions, + data: impl PinInit + 'a, + parent: Option<&'a Device>, + ) -> impl PinInit + 'a + where + T: 'a, + { + let mut dev: Option> =3D None; + + try_pin_init!(&this in Self { + data <- Opaque::pin_init(data), + // TODO: make `inner` in-place when enums get supported by pin= -init. + // + // Link: https://github.com/Rust-for-Linux/pin-init/issues/59 + inner: { + // SAFETY: + // - `this` is a valid pointer to `Self`, + // - `data` was properly initialized above. + let data =3D unsafe { &*(*this.as_ptr()).data.get() }; + + let raw =3D RawDeviceRegistration::new(opts, parent, data); + + // FIXME: Work around a bug in rustc, to prevent the follo= wing warning: + // + // "warning: value captured by `dev` is never read." + // + // Link: https://github.com/rust-lang/rust/issues/141615 + let _ =3D dev; + + if let Some(parent) =3D parent { + let devres =3D Devres::new(parent, raw, GFP_KERNEL)?; + + dev =3D Some(devres.access(parent)?.device().into()); + DeviceRegistrationInner::Managed(devres) + } else { + let boxed =3D KBox::pin_init(raw, GFP_KERNEL)?; + + dev =3D Some(boxed.device().into()); + DeviceRegistrationInner::Raw(boxed) + } + }, + // Cache `this_device` within `Self` to avoid having to access= `Devres` in the managed + // case. + this_device: { + // SAFETY: `dev` is guaranteed to be set in the initialize= r of `inner` above. + unsafe { dev.unwrap_unchecked() } + }, + _t: PhantomData, + }) + } + + /// Access the `this_device` field. + pub fn device(&self) -> &Device { + &self.this_device + } + /// Access the additional data stored in this registration. pub fn data(&self) -> &T::RegistrationData { // SAFETY: @@ -120,9 +222,6 @@ pub fn data(&self) -> &T::RegistrationData { #[pinned_drop] impl PinnedDrop for MiscDeviceRegistration { fn drop(self: Pin<&mut Self>) { - // SAFETY: We know that the device is registered by the type invar= iants. - unsafe { bindings::misc_deregister(self.inner.get()) }; - // SAFETY: `self.data` is valid for dropping. unsafe { core::ptr::drop_in_place(self.data.get()) }; } @@ -137,14 +236,13 @@ pub trait MiscDevice: Sized { /// The additional data carried by the [`MiscDeviceRegistration`] for = this [`MiscDevice`]. /// If no additional data is required than the unit type `()` should b= e used. /// - /// This data can be accessed in [`MiscDevice::open()`] using - /// [`MiscDeviceRegistration::data()`]. + /// This data can be accessed in [`MiscDevice::open()`]. type RegistrationData: Sync; =20 /// Called when the misc device is opened. /// /// The returned pointer will be stored as the private data for the fi= le. - fn open(_file: &File, _misc: &MiscDeviceRegistration) -> Result<= Self::Ptr>; + fn open(_file: &File, _misc: &Device, _data: &Self::RegistrationData) = -> Result; =20 /// Called when the misc device is released. fn release(device: Self::Ptr, _file: &File) { @@ -217,17 +315,17 @@ impl MiscdeviceVTable { // SAFETY: // * `misc_open()` ensures that the `struct miscdevice` can't be u= nregistered and freed // during this call to `fops_open`. - // * The `misc_ptr` always points to the `inner` field of a `MiscD= eviceRegistration`. - // * The `MiscDeviceRegistration` is valid until the `struct mi= scdevice` was + // * The `misc_ptr` always points to the `inner` field of a `RawDe= viceRegistration`. + // * The `RawDeviceRegistration` is valid until the `struct mis= cdevice` was // unregistered. - let registration =3D unsafe { &*container_of!(misc_ptr, MiscDevice= Registration, inner) }; + let registration =3D unsafe { &*container_of!(misc_ptr, RawDeviceR= egistration, inner) }; =20 // SAFETY: // * This underlying file is valid for (much longer than) the dura= tion of `T::open`. // * There is no active fdget_pos region on the file on this threa= d. let file =3D unsafe { File::from_raw_file(raw_file) }; =20 - let ptr =3D match T::open(file, registration) { + let ptr =3D match T::open(file, registration.device(), registratio= n.data()) { Ok(ptr) =3D> ptr, Err(err) =3D> return err.to_errno(), }; diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_devi= ce.rs index 843442b0ea1d..b60fd063afa8 100644 --- a/samples/rust/rust_misc_device.rs +++ b/samples/rust/rust_misc_device.rs @@ -198,7 +198,8 @@ fn init(_module: &'static ThisModule) -> impl PinInit { try_pin_init!(Self { _miscdev <- MiscDeviceRegistration::register( options, - Arc::pin_init(new_mutex!(Inner { value: 0_i32 }), GFP_KERN= EL) + Arc::pin_init(new_mutex!(Inner { value: 0_i32 }), GFP_KERN= EL), + None, ), }) } @@ -222,15 +223,15 @@ impl MiscDevice for RustMiscDevice { =20 type RegistrationData =3D Arc>; =20 - fn open(_file: &File, misc: &MiscDeviceRegistration) -> Result>> { - let dev =3D ARef::from(misc.device()); + fn open(_file: &File, misc: &Device, data: &Self::RegistrationData) ->= Result>> { + let dev =3D ARef::from(misc); =20 dev_info!(dev, "Opening Rust Misc Device Sample\n"); =20 KBox::try_pin_init( try_pin_init! { RustMiscDevice { - shared: misc.data().clone(), + shared: data.clone(), unique <- new_mutex!(Inner { value: 0_i32 }), dev: dev, } --=20 2.49.0