* [PATCH 0/4] rust: dma: tie DMA allocations to the device's bound lifetime
@ 2026-08-30 19:37 Danilo Krummrich
2026-08-30 19:37 ` [PATCH 1/4] rust: debugfs: drop 'static bound from ScopedDir file creation methods Danilo Krummrich
` (3 more replies)
0 siblings, 4 replies; 16+ messages in thread
From: Danilo Krummrich @ 2026-08-30 19:37 UTC (permalink / raw)
To: dakr, abdiel.janulgue, daniel.almeida, robin.murphy, a.hindborg,
gregkh, rafael, aliceryhl, acourbot, ojeda, boqun, gary,
bjorn3_gh, lossin, tmgross, tamird, work, mmaurer
Cc: driver-core, nova-gpu, dri-devel, linux-kernel, rust-for-linux
DMA allocations carry device resources (e.g. IOMMU mappings) that must not
outlive the device's bound lifetime. Add lifetime parameters to the DMA
allocation types (Coherent, CoherentBox, CoherentHandle) to enforce at compile
time that they are freed before the device is unbound.
Since DMA types with lifetime parameters are exposed through debugfs in the
nova-core driver, first drop the unnecessary T: 'static bound from the debugfs
ScopedDir file creation methods by formalizing a type invariant on FileOps.
Danilo Krummrich (4):
rust: debugfs: drop 'static bound from ScopedDir file creation methods
rust: dma: tie CoherentHandle to the device's bound lifetime
samples: rust_dma: separate driver type from driver data
rust: dma: tie Coherent and CoherentBox to the device's bound lifetime
drivers/gpu/nova-core/falcon.rs | 2 +-
drivers/gpu/nova-core/fb.rs | 4 +-
drivers/gpu/nova-core/firmware/booter.rs | 2 +-
drivers/gpu/nova-core/firmware/fsp.rs | 8 +-
.../nova-core/firmware/fwsec/bootloader.rs | 12 +-
drivers/gpu/nova-core/firmware/gsp.rs | 14 +-
drivers/gpu/nova-core/firmware/riscv.rs | 8 +-
drivers/gpu/nova-core/fsp.rs | 20 +--
drivers/gpu/nova-core/gpu.rs | 4 +-
drivers/gpu/nova-core/gsp.rs | 30 ++--
drivers/gpu/nova-core/gsp/boot.rs | 10 +-
drivers/gpu/nova-core/gsp/cmdq.rs | 35 +++--
drivers/gpu/nova-core/gsp/commands.rs | 2 +-
drivers/gpu/nova-core/gsp/fw.rs | 12 +-
drivers/gpu/nova-core/gsp/hal.rs | 14 +-
drivers/gpu/nova-core/gsp/hal/gh100.rs | 16 +-
drivers/gpu/nova-core/gsp/hal/tu102.rs | 34 ++---
drivers/gpu/nova-core/gsp/sequencer.rs | 6 +-
rust/kernel/debugfs.rs | 26 +---
rust/kernel/debugfs/entry.rs | 4 +-
rust/kernel/debugfs/file_ops.rs | 52 ++++---
rust/kernel/dma.rs | 141 +++++++++---------
rust/kernel/uaccess.rs | 4 +-
samples/rust/rust_dma.rs | 30 ++--
24 files changed, 241 insertions(+), 249 deletions(-)
base-commit: 45c13f3f9e3bb15fd89ff2864c6f627a3b4b4229
--
2.55.0
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/4] rust: debugfs: drop 'static bound from ScopedDir file creation methods
2026-08-30 19:37 [PATCH 0/4] rust: dma: tie DMA allocations to the device's bound lifetime Danilo Krummrich
@ 2026-08-30 19:37 ` Danilo Krummrich
2026-08-30 19:53 ` sashiko-bot
2026-09-03 13:12 ` Gary Guo
2026-08-30 19:37 ` [PATCH 2/4] rust: dma: tie CoherentHandle to the device's bound lifetime Danilo Krummrich
` (2 subsequent siblings)
3 siblings, 2 replies; 16+ messages in thread
From: Danilo Krummrich @ 2026-08-30 19:37 UTC (permalink / raw)
To: dakr, abdiel.janulgue, daniel.almeida, robin.murphy, a.hindborg,
gregkh, rafael, aliceryhl, acourbot, ojeda, boqun, gary,
bjorn3_gh, lossin, tmgross, tamird, work, mmaurer
Cc: driver-core, nova-gpu, dri-devel, linux-kernel, rust-for-linux
Drop the T: 'static bound from ScopedDir's file creation methods
(read_binary_file(), read_only_file(), etc.) to support registering
debugfs files backed by types that contain non-'static references, such
as dma::Coherent<'a, T>.
The previous 'static bound existed because ScopedDir::create_file() took
&'static FileOps<T>, and &'static requires T: 'static for well-
formedness. However, this was overly conservative; FileOps instances are
always associated consts residing in static storage, so the pointer
passed to the C debugfs API is always valid for the file's lifetime.
Formalize this as a type invariant on FileOps. All instances reside in
static storage, enforced by requiring FileOps::new() to only be used in
const/static items. Replace the Deref impl with an explicit fops()
method that returns &'static bindings::file_operations, justified by the
type invariant.
With this, ScopedDir::create_file() takes &FileOps<T> (no 'static),
preserving the generic type safety (T links the fops to the data type)
while allowing non-'static T.
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
rust/kernel/debugfs.rs | 26 +++++------------
rust/kernel/debugfs/entry.rs | 4 +--
rust/kernel/debugfs/file_ops.rs | 52 +++++++++++++++++++--------------
3 files changed, 39 insertions(+), 43 deletions(-)
diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
index d7b8014a6474..2beb55d444ca 100644
--- a/rust/kernel/debugfs.rs
+++ b/rust/kernel/debugfs.rs
@@ -538,7 +538,7 @@ pub fn dir<'dir2>(&'dir2 self, name: &CStr) -> ScopedDir<'data, 'dir2> {
}
}
- fn create_file<T: Sync>(&self, name: &CStr, data: &'data T, vtable: &'static FileOps<T>) {
+ fn create_file<T: Sync>(&self, name: &CStr, data: &'data T, vtable: &FileOps<T>) {
#[cfg(CONFIG_DEBUG_FS)]
core::mem::forget(Entry::file(name, &self.entry, data, vtable));
}
@@ -550,7 +550,7 @@ fn create_file<T: Sync>(&self, name: &CStr, data: &'data T, vtable: &'static Fil
/// This function does not produce an owning handle to the file. The created
/// file is removed when the [`Scope`] that this directory belongs
/// to is dropped.
- pub fn read_only_file<T: Writer + Send + Sync + 'static>(&self, name: &CStr, data: &'data T) {
+ pub fn read_only_file<T: Writer + Send + Sync>(&self, name: &CStr, data: &'data T) {
self.create_file(name, data, &T::FILE_OPS)
}
@@ -560,11 +560,7 @@ pub fn read_only_file<T: Writer + Send + Sync + 'static>(&self, name: &CStr, dat
///
/// This function does not produce an owning handle to the file. The created file is removed
/// when the [`Scope`] that this directory belongs to is dropped.
- pub fn read_binary_file<T: BinaryWriter + Send + Sync + 'static>(
- &self,
- name: &CStr,
- data: &'data T,
- ) {
+ pub fn read_binary_file<T: BinaryWriter + Send + Sync>(&self, name: &CStr, data: &'data T) {
self.create_file(name, data, &T::FILE_OPS)
}
@@ -596,11 +592,7 @@ pub fn read_callback_file<T, F>(&self, name: &CStr, data: &'data T, _f: &'static
/// This function does not produce an owning handle to the file. The created
/// file is removed when the [`Scope`] that this directory belongs
/// to is dropped.
- pub fn read_write_file<T: Writer + Reader + Send + Sync + 'static>(
- &self,
- name: &CStr,
- data: &'data T,
- ) {
+ pub fn read_write_file<T: Writer + Reader + Send + Sync>(&self, name: &CStr, data: &'data T) {
let vtable = &<T as ReadWriteFile<_>>::FILE_OPS;
self.create_file(name, data, vtable)
}
@@ -612,7 +604,7 @@ pub fn read_write_file<T: Writer + Reader + Send + Sync + 'static>(
///
/// This function does not produce an owning handle to the file. The created file is removed
/// when the [`Scope`] that this directory belongs to is dropped.
- pub fn read_write_binary_file<T: BinaryWriter + BinaryReader + Send + Sync + 'static>(
+ pub fn read_write_binary_file<T: BinaryWriter + BinaryReader + Send + Sync>(
&self,
name: &CStr,
data: &'data T,
@@ -655,7 +647,7 @@ pub fn read_write_callback_file<T, F, W>(
/// This function does not produce an owning handle to the file. The created
/// file is removed when the [`Scope`] that this directory belongs
/// to is dropped.
- pub fn write_only_file<T: Reader + Send + Sync + 'static>(&self, name: &CStr, data: &'data T) {
+ pub fn write_only_file<T: Reader + Send + Sync>(&self, name: &CStr, data: &'data T) {
let vtable = &<T as WriteFile<_>>::FILE_OPS;
self.create_file(name, data, vtable)
}
@@ -666,11 +658,7 @@ pub fn write_only_file<T: Reader + Send + Sync + 'static>(&self, name: &CStr, da
///
/// This function does not produce an owning handle to the file. The created file is removed
/// when the [`Scope`] that this directory belongs to is dropped.
- pub fn write_binary_file<T: BinaryReader + Send + Sync + 'static>(
- &self,
- name: &CStr,
- data: &'data T,
- ) {
+ pub fn write_binary_file<T: BinaryReader + Send + Sync>(&self, name: &CStr, data: &'data T) {
self.create_file(name, data, &T::FILE_OPS)
}
diff --git a/rust/kernel/debugfs/entry.rs b/rust/kernel/debugfs/entry.rs
index 46aad64896ec..88a870d8c295 100644
--- a/rust/kernel/debugfs/entry.rs
+++ b/rust/kernel/debugfs/entry.rs
@@ -74,7 +74,7 @@ pub(crate) unsafe fn dynamic_file<T>(
parent.as_ptr(),
core::ptr::from_ref(data) as *mut c_void,
core::ptr::null(),
- &**file_ops,
+ file_ops.fops(),
)
};
@@ -127,7 +127,7 @@ pub(crate) fn file<T>(
parent.as_ptr(),
core::ptr::from_ref(data) as *mut c_void,
core::ptr::null(),
- &**file_ops,
+ file_ops.fops(),
)
};
diff --git a/rust/kernel/debugfs/file_ops.rs b/rust/kernel/debugfs/file_ops.rs
index f15908f71c4a..7e1dd8c75ad9 100644
--- a/rust/kernel/debugfs/file_ops.rs
+++ b/rust/kernel/debugfs/file_ops.rs
@@ -20,14 +20,12 @@
use core::marker::PhantomData;
-#[cfg(CONFIG_DEBUG_FS)]
-use core::ops::Deref;
-
-/// # Invariant
+/// # Invariants
///
-/// `FileOps<T>` will always contain an `operations` which is safe to use for a file backed
-/// off an inode which has a pointer to a `T` in its private data that is safe to convert
-/// into a reference.
+/// - `FileOps<T>` will always contain an `operations` which is safe to use for a file backed
+/// off an inode which has a pointer to a `T` in its private data that is safe to convert
+/// into a reference.
+/// - Every instance of `FileOps<T>` resides in static storage.
pub(super) struct FileOps<T> {
#[cfg(CONFIG_DEBUG_FS)]
operations: bindings::file_operations,
@@ -39,9 +37,13 @@ pub(super) struct FileOps<T> {
impl<T> FileOps<T> {
/// # Safety
///
- /// The caller asserts that the provided `operations` is safe to use for a file whose
- /// inode has a pointer to `T` in its private data that is safe to convert into a reference.
+ /// - The caller asserts that the provided `operations` is safe to use for a file whose
+ /// inode has a pointer to `T` in its private data that is safe to convert into a reference.
+ /// - Must only be used to initialize a `const` or `static` item, to uphold the type invariant
+ /// that all `FileOps` instances reside in static storage.
const unsafe fn new(operations: bindings::file_operations, mode: u16) -> Self {
+ // INVARIANT: The caller is required to only use this in a `const` or `static` item,
+ // ensuring that all `FileOps` instances reside in static storage.
Self {
#[cfg(CONFIG_DEBUG_FS)]
operations,
@@ -65,11 +67,11 @@ pub(super) const fn adapt(&self) -> &FileOps<T::Inner> {
}
#[cfg(CONFIG_DEBUG_FS)]
-impl<T> Deref for FileOps<T> {
- type Target = bindings::file_operations;
-
- fn deref(&self) -> &Self::Target {
- &self.operations
+impl<T> FileOps<T> {
+ /// Returns a `'static` reference to the inner `file_operations`.
+ pub(crate) fn fops(&self) -> &'static bindings::file_operations {
+ // SAFETY: By the type invariant, `self` resides in static storage.
+ unsafe { core::mem::transmute(&self.operations) }
}
}
@@ -138,9 +140,10 @@ impl<T: Writer + Sync> ReadFile<T> for T {
..pin_init::zeroed()
};
// SAFETY: `operations` is all stock `seq_file` implementations except for `writer_open`.
- // `open`'s only requirement beyond what is provided to all open functions is that the
- // inode's data pointer must point to a `T` that will outlive it, which matches the
- // `FileOps` requirements.
+ // - `open`'s only requirement beyond what is provided to all open functions is that the
+ // inode's data pointer must point to a `T` that will outlive it, which matches the
+ // `FileOps` requirements.
+ // - This is a `const` item, satisfying the static storage invariant.
unsafe { FileOps::new(operations, 0o400) }
};
}
@@ -194,9 +197,10 @@ impl<T: Writer + Reader + Sync> ReadWriteFile<T> for T {
// `writer_open`'s only requirement beyond what is provided to all open functions is that
// the inode's data pointer must point to a `T` that will outlive it, which matches the
// `FileOps` requirements.
- // `write` only requires that the file's private data pointer points to `seq_file`
- // which points to a `T` that will outlive it, which matches what `writer_open`
- // provides.
+ // - `write` only requires that the file's private data pointer points to `seq_file`
+ // which points to a `T` that will outlive it, which matches what `writer_open`
+ // provides.
+ // - This is a `const` item, satisfying the static storage invariant.
unsafe { FileOps::new(operations, 0o600) }
};
}
@@ -245,10 +249,11 @@ impl<T: Reader + Sync> WriteFile<T> for T {
..pin_init::zeroed()
};
// SAFETY:
- // * `write_only_open` populates the file private data with the inode private data
- // * `write_only_write`'s only requirement is that the private data of the file point to
+ // - `write_only_open` populates the file private data with the inode private data
+ // - `write_only_write`'s only requirement is that the private data of the file point to
// a `T` and be legal to convert to a shared reference, which `write_only_open`
// satisfies.
+ // - This is a `const` item, satisfying the static storage invariant.
unsafe { FileOps::new(operations, 0o200) }
};
}
@@ -303,6 +308,7 @@ impl<T: BinaryWriter + Sync> BinaryReadFile<T> for T {
// corresponding `struct file`.
// - `blob_read()` re-creates a reference to `T` from the `struct file`'s private data.
// - `default_llseek()` does not access the `struct file`'s private data.
+ // - This is a `const` item, satisfying the static storage invariant.
unsafe { FileOps::new(operations, 0o400) }
};
}
@@ -357,6 +363,7 @@ impl<T: BinaryReader + Sync> BinaryWriteFile<T> for T {
// corresponding `struct file`.
// - `blob_write()` re-creates a reference to `T` from the `struct file`'s private data.
// - `default_llseek()` does not access the `struct file`'s private data.
+ // - This is a `const` item, satisfying the static storage invariant.
unsafe { FileOps::new(operations, 0o200) }
};
}
@@ -383,6 +390,7 @@ impl<T: BinaryWriter + BinaryReader + Sync> BinaryReadWriteFile<T> for T {
// - `blob_read()` re-creates a reference to `T` from the `struct file`'s private data.
// - `blob_write()` re-creates a reference to `T` from the `struct file`'s private data.
// - `default_llseek()` does not access the `struct file`'s private data.
+ // - This is a `const` item, satisfying the static storage invariant.
unsafe { FileOps::new(operations, 0o600) }
};
}
--
2.55.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/4] rust: dma: tie CoherentHandle to the device's bound lifetime
2026-08-30 19:37 [PATCH 0/4] rust: dma: tie DMA allocations to the device's bound lifetime Danilo Krummrich
2026-08-30 19:37 ` [PATCH 1/4] rust: debugfs: drop 'static bound from ScopedDir file creation methods Danilo Krummrich
@ 2026-08-30 19:37 ` Danilo Krummrich
2026-09-03 13:12 ` Gary Guo
2026-08-30 19:37 ` [PATCH 3/4] samples: rust_dma: separate driver type from driver data Danilo Krummrich
2026-08-30 19:37 ` [PATCH 4/4] rust: dma: tie Coherent and CoherentBox to the device's bound lifetime Danilo Krummrich
3 siblings, 1 reply; 16+ messages in thread
From: Danilo Krummrich @ 2026-08-30 19:37 UTC (permalink / raw)
To: dakr, abdiel.janulgue, daniel.almeida, robin.murphy, a.hindborg,
gregkh, rafael, aliceryhl, acourbot, ojeda, boqun, gary,
bjorn3_gh, lossin, tmgross, tamird, work, mmaurer
Cc: driver-core, nova-gpu, dri-devel, linux-kernel, rust-for-linux
Add a lifetime parameter to CoherentHandle that ties the DMA allocation
to the device's bound scope, ensuring it is freed before the device is
unbound.
DMA allocations carry device resources (e.g. IOMMU mappings) that must
not outlive the device's bound lifetime. Without a lifetime parameter,
there was no compile-time enforcement that a CoherentHandle is dropped
before the device is unbound.
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
drivers/gpu/nova-core/fb.rs | 2 +-
rust/kernel/dma.rs | 20 ++++++++++----------
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/nova-core/fb.rs b/drivers/gpu/nova-core/fb.rs
index 1576399389b1..9ef232a73dee 100644
--- a/drivers/gpu/nova-core/fb.rs
+++ b/drivers/gpu/nova-core/fb.rs
@@ -49,7 +49,7 @@ pub(crate) struct SysmemFlush<'sys> {
device: &'sys device::Device,
bar: Bar0<'sys>,
/// Keep the page alive as long as we need it.
- page: CoherentHandle,
+ page: CoherentHandle<'sys>,
}
impl<'sys> SysmemFlush<'sys> {
diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
index 2ce09f8e90c6..79f453e9ec0b 100644
--- a/rust/kernel/dma.rs
+++ b/rust/kernel/dma.rs
@@ -996,15 +996,15 @@ fn write_to_slice(
/// - `size` is the allocation size in bytes as passed to `dma_alloc_attrs`.
/// - `dma_attrs` contains the attributes used for the allocation, always including
/// `DMA_ATTR_NO_KERNEL_MAPPING`.
-pub struct CoherentHandle {
- dev: ARef<device::Device>,
+pub struct CoherentHandle<'a> {
+ dev: &'a device::Device<Bound>,
dma_addr: DmaAddress,
cpu_handle: NonNull<c_void>,
size: usize,
dma_attrs: Attrs,
}
-impl CoherentHandle {
+impl<'a> CoherentHandle<'a> {
/// Allocates `size` bytes of coherent DMA memory without creating a kernel virtual mapping.
///
/// Additional DMA attributes may be passed via `dma_attrs`; `DMA_ATTR_NO_KERNEL_MAPPING` is
@@ -1012,7 +1012,7 @@ impl CoherentHandle {
///
/// Returns `EINVAL` if `size` is zero, `ENOMEM` if the allocation fails.
pub fn alloc_with_attrs(
- dev: &device::Device<Bound>,
+ dev: &'a device::Device<Bound>,
size: usize,
gfp_flags: kernel::alloc::Flags,
dma_attrs: Attrs,
@@ -1038,9 +1038,9 @@ pub fn alloc_with_attrs(
// INVARIANT: `cpu_handle` is the opaque handle from a successful `dma_alloc_attrs` call
// with `DMA_ATTR_NO_KERNEL_MAPPING`, `dma_addr` is the corresponding DMA address,
- // and we hold a refcounted reference to the device.
+ // and `dev` is a valid reference to a bound device that outlives this allocation.
Ok(Self {
- dev: dev.into(),
+ dev,
dma_addr,
cpu_handle,
size,
@@ -1051,7 +1051,7 @@ pub fn alloc_with_attrs(
/// Allocates `size` bytes of coherent DMA memory without creating a kernel virtual mapping.
#[inline]
pub fn alloc(
- dev: &device::Device<Bound>,
+ dev: &'a device::Device<Bound>,
size: usize,
gfp_flags: kernel::alloc::Flags,
) -> Result<Self> {
@@ -1073,7 +1073,7 @@ pub fn size(&self) -> usize {
}
}
-impl Drop for CoherentHandle {
+impl Drop for CoherentHandle<'_> {
fn drop(&mut self) {
// SAFETY: All values are valid by the type invariants on `CoherentHandle`.
// `cpu_handle` is the opaque handle from `dma_alloc_attrs` and is passed back unchanged.
@@ -1091,12 +1091,12 @@ fn drop(&mut self) {
// SAFETY: `CoherentHandle` only holds a device reference, a DMA address, an opaque CPU handle,
// and a size. None of these are tied to a specific thread.
-unsafe impl Send for CoherentHandle {}
+unsafe impl Send for CoherentHandle<'_> {}
// SAFETY: `CoherentHandle` provides no CPU access to the underlying allocation. The only
// operations on `&CoherentHandle` are reading the DMA address and size, both of which are
// plain `Copy` values.
-unsafe impl Sync for CoherentHandle {}
+unsafe impl Sync for CoherentHandle<'_> {}
/// View type for `Coherent`.
///
--
2.55.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/4] samples: rust_dma: separate driver type from driver data
2026-08-30 19:37 [PATCH 0/4] rust: dma: tie DMA allocations to the device's bound lifetime Danilo Krummrich
2026-08-30 19:37 ` [PATCH 1/4] rust: debugfs: drop 'static bound from ScopedDir file creation methods Danilo Krummrich
2026-08-30 19:37 ` [PATCH 2/4] rust: dma: tie CoherentHandle to the device's bound lifetime Danilo Krummrich
@ 2026-08-30 19:37 ` Danilo Krummrich
2026-08-30 19:57 ` sashiko-bot
2026-09-03 13:13 ` Gary Guo
2026-08-30 19:37 ` [PATCH 4/4] rust: dma: tie Coherent and CoherentBox to the device's bound lifetime Danilo Krummrich
3 siblings, 2 replies; 16+ messages in thread
From: Danilo Krummrich @ 2026-08-30 19:37 UTC (permalink / raw)
To: dakr, abdiel.janulgue, daniel.almeida, robin.murphy, a.hindborg,
gregkh, rafael, aliceryhl, acourbot, ojeda, boqun, gary,
bjorn3_gh, lossin, tmgross, tamird, work, mmaurer
Cc: driver-core, nova-gpu, dri-devel, linux-kernel, rust-for-linux
Split DmaSampleDriver into a driver type and a separate DmaSampleData
struct for the driver's bus device private data, using
DmaSampleData<'bound> as the Driver::Data<'bound> associated type.
Store a &'bound pci::Device<Bound> reference instead of an
ARef<pci::Device>, tying the data to the device's bound scope.
This prepares for adding a lifetime parameter to dma::Coherent, which
requires the data type to carry a lifetime.
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
samples/rust/rust_dma.rs | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
index bd60034ded23..0fac9d4ae566 100644
--- a/samples/rust/rust_dma.rs
+++ b/samples/rust/rust_dma.rs
@@ -5,7 +5,10 @@
//! To make this driver probe, QEMU must be run with `-device pci-testdev`.
use kernel::{
- device::Core,
+ device::{
+ Bound,
+ Core, //
+ },
dma::{
Coherent,
DataDirection,
@@ -23,13 +26,14 @@
scatterlist::{
Owned,
SGTable, //
- },
- sync::aref::ARef, //
+ }, //
};
+struct DmaSampleDriver;
+
#[pin_data(PinnedDrop)]
-struct DmaSampleDriver {
- pdev: ARef<pci::Device>,
+struct DmaSampleData<'bound> {
+ pdev: &'bound pci::Device<Bound>,
ca: Coherent<[MyStruct]>,
#[pin]
sgt: SGTable<Owned<VVec<u8>>>,
@@ -67,13 +71,13 @@ unsafe impl kernel::transmute::FromBytes for MyStruct {}
impl pci::Driver for DmaSampleDriver {
type IdInfo = ();
- type Data<'bound> = Self;
+ type Data<'bound> = DmaSampleData<'bound>;
const ID_TABLE: pci::IdTable<Self::IdInfo> = &PCI_TABLE;
fn probe<'bound>(
pdev: &'bound pci::Device<Core<'_>>,
_info: Option<&'bound Self::IdInfo>,
- ) -> impl PinInit<Self, Error> + 'bound {
+ ) -> impl PinInit<Self::Data<'bound>, Error> + 'bound {
pin_init::pin_init_scope(move || {
dev_info!(pdev, "Probe DMA test driver.\n");
@@ -94,8 +98,8 @@ fn probe<'bound>(
let sgt = SGTable::new(pdev.as_ref(), pages, DataDirection::ToDevice, GFP_KERNEL);
- Ok(try_pin_init!(Self {
- pdev: pdev.into(),
+ Ok(try_pin_init!(Self::Data {
+ pdev,
ca,
sgt <- sgt,
}))
@@ -103,7 +107,7 @@ fn probe<'bound>(
}
}
-impl DmaSampleDriver {
+impl DmaSampleData<'_> {
fn check_dma(&self) {
for (i, value) in TEST_VALUES.into_iter().enumerate() {
let val0 = io_read!(self.ca, [panic: i].h);
@@ -116,7 +120,7 @@ fn check_dma(&self) {
}
#[pinned_drop]
-impl PinnedDrop for DmaSampleDriver {
+impl PinnedDrop for DmaSampleData<'_> {
fn drop(self: Pin<&mut Self>) {
dev_info!(self.pdev, "Unload DMA test driver.\n");
--
2.55.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 4/4] rust: dma: tie Coherent and CoherentBox to the device's bound lifetime
2026-08-30 19:37 [PATCH 0/4] rust: dma: tie DMA allocations to the device's bound lifetime Danilo Krummrich
` (2 preceding siblings ...)
2026-08-30 19:37 ` [PATCH 3/4] samples: rust_dma: separate driver type from driver data Danilo Krummrich
@ 2026-08-30 19:37 ` Danilo Krummrich
2026-08-30 19:47 ` sashiko-bot
2026-09-03 13:20 ` Gary Guo
3 siblings, 2 replies; 16+ messages in thread
From: Danilo Krummrich @ 2026-08-30 19:37 UTC (permalink / raw)
To: dakr, abdiel.janulgue, daniel.almeida, robin.murphy, a.hindborg,
gregkh, rafael, aliceryhl, acourbot, ojeda, boqun, gary,
bjorn3_gh, lossin, tmgross, tamird, work, mmaurer
Cc: driver-core, nova-gpu, dri-devel, linux-kernel, rust-for-linux
Add a lifetime parameter to Coherent and CoherentBox that ties the DMA
allocation to the device's bound scope, ensuring it is freed before the
device is unbound.
DMA allocations carry device resources (e.g. IOMMU mappings) that must
not outlive the device's bound lifetime. Without a lifetime parameter,
there was no compile-time enforcement that a Coherent or CoherentBox is
dropped before the device is unbound.
Propagate the new lifetime parameter through all users.
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
drivers/gpu/nova-core/falcon.rs | 2 +-
drivers/gpu/nova-core/fb.rs | 2 +-
drivers/gpu/nova-core/firmware/booter.rs | 2 +-
drivers/gpu/nova-core/firmware/fsp.rs | 8 +-
.../nova-core/firmware/fwsec/bootloader.rs | 12 +-
drivers/gpu/nova-core/firmware/gsp.rs | 14 +-
drivers/gpu/nova-core/firmware/riscv.rs | 8 +-
drivers/gpu/nova-core/fsp.rs | 20 +--
drivers/gpu/nova-core/gpu.rs | 4 +-
drivers/gpu/nova-core/gsp.rs | 30 ++---
drivers/gpu/nova-core/gsp/boot.rs | 10 +-
drivers/gpu/nova-core/gsp/cmdq.rs | 35 +++--
drivers/gpu/nova-core/gsp/commands.rs | 2 +-
drivers/gpu/nova-core/gsp/fw.rs | 12 +-
drivers/gpu/nova-core/gsp/hal.rs | 14 +-
drivers/gpu/nova-core/gsp/hal/gh100.rs | 16 +--
drivers/gpu/nova-core/gsp/hal/tu102.rs | 34 ++---
drivers/gpu/nova-core/gsp/sequencer.rs | 6 +-
rust/kernel/dma.rs | 121 +++++++++---------
rust/kernel/uaccess.rs | 4 +-
samples/rust/rust_dma.rs | 4 +-
21 files changed, 176 insertions(+), 184 deletions(-)
diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
index 65cb12d26e2b..15eba039cb69 100644
--- a/drivers/gpu/nova-core/falcon.rs
+++ b/drivers/gpu/nova-core/falcon.rs
@@ -506,7 +506,7 @@ pub(crate) fn pio_load<F: FalconFirmware<Target = E> + FalconPioLoadable>(
/// `sec` is set if the loaded firmware is expected to run in secure mode.
fn dma_wr(
&self,
- dma_obj: &Coherent<[u8]>,
+ dma_obj: &Coherent<'_, [u8]>,
target_mem: FalconMem,
load_offsets: FalconDmaLoadTarget,
) -> Result {
diff --git a/drivers/gpu/nova-core/fb.rs b/drivers/gpu/nova-core/fb.rs
index 9ef232a73dee..b3a6ab8b57a6 100644
--- a/drivers/gpu/nova-core/fb.rs
+++ b/drivers/gpu/nova-core/fb.rs
@@ -177,7 +177,7 @@ impl FbRanges {
pub(crate) fn new(
chipset: Chipset,
bar: Bar0<'_>,
- gsp_fw: &GspFirmware,
+ gsp_fw: &GspFirmware<'_>,
vgpu_state: VgpuState,
) -> Result<Self> {
let hal = hal::fb_hal(chipset);
diff --git a/drivers/gpu/nova-core/firmware/booter.rs b/drivers/gpu/nova-core/firmware/booter.rs
index dc071edba331..aa4458bb3312 100644
--- a/drivers/gpu/nova-core/firmware/booter.rs
+++ b/drivers/gpu/nova-core/firmware/booter.rs
@@ -186,7 +186,7 @@ pub(crate) fn run<T>(
&self,
dev: &device::Device<device::Bound>,
sec2_falcon: &Falcon<'_, Sec2>,
- wpr_meta: &Coherent<T>,
+ wpr_meta: &Coherent<'_, T>,
) -> Result {
sec2_falcon.reset()?;
sec2_falcon.load(self)?;
diff --git a/drivers/gpu/nova-core/firmware/fsp.rs b/drivers/gpu/nova-core/firmware/fsp.rs
index 5462e318410a..d47b1d2a1030 100644
--- a/drivers/gpu/nova-core/firmware/fsp.rs
+++ b/drivers/gpu/nova-core/firmware/fsp.rs
@@ -39,15 +39,15 @@ pub(crate) struct FmcSignatures {
pub(crate) signature: [u8; FSP_SIG_SIZE],
}
-pub(crate) struct FspFirmware {
+pub(crate) struct FspFirmware<'a> {
/// FMC firmware image data
- pub(crate) fmc_image: Coherent<[u8]>,
+ pub(crate) fmc_image: Coherent<'a, [u8]>,
/// FMC firmware signatures.
pub(crate) fmc_sigs: KBox<FmcSignatures>,
}
-impl FspFirmware {
- pub(crate) fn new(dev: &device::Device<device::Bound>, chipset: Chipset) -> Result<Self> {
+impl<'a> FspFirmware<'a> {
+ pub(crate) fn new(dev: &'a device::Device<device::Bound>, chipset: Chipset) -> Result<Self> {
let fw = request_tlv(dev, chipset, "fmc")?;
let tlv = Tlv::new(fw.data())?;
dev_dbg!(dev, "loaded fsp firmware v{}\n", tlv.get_string(b"VERS")?);
diff --git a/drivers/gpu/nova-core/firmware/fwsec/bootloader.rs b/drivers/gpu/nova-core/firmware/fwsec/bootloader.rs
index ec4d92317a93..06a7936a8c5e 100644
--- a/drivers/gpu/nova-core/firmware/fwsec/bootloader.rs
+++ b/drivers/gpu/nova-core/firmware/fwsec/bootloader.rs
@@ -98,9 +98,9 @@ unsafe impl AsBytes for BootloaderDmemDescV2 {}
/// Wrapper for [`FwsecFirmware`] that includes the bootloader performing the actual load
/// operation.
-pub(crate) struct FwsecFirmwareWithBl {
+pub(crate) struct FwsecFirmwareWithBl<'a> {
/// DMA object the bootloader will copy the firmware from.
- _firmware_dma: Coherent<[u8]>,
+ _firmware_dma: Coherent<'a, [u8]>,
/// Code of the bootloader to be loaded into non-secure IMEM.
ucode: KVec<u8>,
/// Descriptor to be loaded into DMEM for the bootloader to read.
@@ -113,12 +113,12 @@ pub(crate) struct FwsecFirmwareWithBl {
start_tag: u16,
}
-impl FwsecFirmwareWithBl {
+impl<'a> FwsecFirmwareWithBl<'a> {
/// Loads the bootloader firmware for `dev` and `chipset`, and wrap `firmware` so it can be
/// loaded using it.
pub(crate) fn new(
firmware: FwsecFirmware,
- dev: &Device<device::Bound>,
+ dev: &'a Device<device::Bound>,
chipset: Chipset,
) -> Result<Self> {
let fw = request_tlv(dev, chipset, "gen_bootloader")?;
@@ -272,7 +272,7 @@ pub(crate) fn run(
}
}
-impl FalconFirmware for FwsecFirmwareWithBl {
+impl FalconFirmware for FwsecFirmwareWithBl<'_> {
type Target = Gsp;
fn brom_params(&self) -> FalconBromParams {
@@ -286,7 +286,7 @@ fn boot_addr(&self) -> u32 {
}
}
-impl FalconPioLoadable for FwsecFirmwareWithBl {
+impl FalconPioLoadable for FwsecFirmwareWithBl<'_> {
fn imem_sec_load_params(&self) -> Option<FalconPioImemLoadTarget<'_>> {
None
}
diff --git a/drivers/gpu/nova-core/firmware/gsp.rs b/drivers/gpu/nova-core/firmware/gsp.rs
index e8f9491e84cc..22d1f9329c9f 100644
--- a/drivers/gpu/nova-core/firmware/gsp.rs
+++ b/drivers/gpu/nova-core/firmware/gsp.rs
@@ -44,7 +44,7 @@
/// Each page is 4KB, each entry is 8 bytes (64-bit DMA address).
/// Also known as "Radix3" firmware.
#[pin_data]
-pub(crate) struct GspFirmware {
+pub(crate) struct GspFirmware<'a> {
/// The GSP firmware inside a [`VVec`], device-mapped via a SG table.
#[pin]
fw: SGTable<Owned<VVec<u8>>>,
@@ -55,19 +55,19 @@ pub(crate) struct GspFirmware {
#[pin]
level1: SGTable<Owned<VVec<u8>>>,
/// Level 0 page table (single 4KB page) with one entry: DMA address of first level 1 page.
- level0: Coherent<[u64]>,
+ level0: Coherent<'a, [u64]>,
/// Size in bytes of the firmware contained in [`Self::fw`].
pub(crate) size: usize,
/// Device-mapped GSP signatures matching the GPU's [`Chipset`].
- pub(crate) signatures: Coherent<[u8]>,
+ pub(crate) signatures: Coherent<'a, [u8]>,
/// GSP bootloader, verifies the GSP firmware before loading and running it.
- pub(crate) bootloader: RiscvFirmware,
+ pub(crate) bootloader: RiscvFirmware<'a>,
}
-impl GspFirmware {
+impl<'a> GspFirmware<'a> {
/// Loads the GSP firmware binaries, map them into `dev`'s address-space, and creates the page
/// tables expected by the GSP bootloader to load it.
- pub(crate) fn new<'a>(
+ pub(crate) fn new(
dev: &'a device::Device<device::Bound>,
chipset: Chipset,
) -> impl PinInit<Self, Error> + 'a {
@@ -120,7 +120,7 @@ pub(crate) fn new<'a>(
// Create level 0 page table data and fill its first entry with the level 1
// table.
- let mut level0 = CoherentBox::<[u64]>::zeroed_slice(
+ let mut level0 = CoherentBox::<'_, [u64]>::zeroed_slice(
dev,
GSP_PAGE_SIZE / size_of::<u64>(),
GFP_KERNEL
diff --git a/drivers/gpu/nova-core/firmware/riscv.rs b/drivers/gpu/nova-core/firmware/riscv.rs
index 1403f05a7305..f05cfb1c65da 100644
--- a/drivers/gpu/nova-core/firmware/riscv.rs
+++ b/drivers/gpu/nova-core/firmware/riscv.rs
@@ -13,7 +13,7 @@
use crate::firmware::tlv::Tlv;
/// A parsed firmware for a RISC-V core, ready to be loaded and run.
-pub(crate) struct RiscvFirmware {
+pub(crate) struct RiscvFirmware<'a> {
/// Offset at which the code starts in the firmware image.
pub(crate) code_offset: u32,
/// Offset at which the data starts in the firmware image.
@@ -23,12 +23,12 @@ pub(crate) struct RiscvFirmware {
/// Application version.
pub(crate) app_version: u32,
/// Device-mapped firmware image.
- pub(crate) ucode: Coherent<[u8]>,
+ pub(crate) ucode: Coherent<'a, [u8]>,
}
-impl RiscvFirmware {
+impl<'a> RiscvFirmware<'a> {
/// Parses the RISC-V firmware image contained in `fw`.
- pub(crate) fn new(dev: &device::Device<device::Bound>, fw: &Firmware) -> Result<Self> {
+ pub(crate) fn new(dev: &'a device::Device<device::Bound>, fw: &Firmware) -> Result<Self> {
let tlv = Tlv::new(fw.data())?;
dev_dbg!(
dev,
diff --git a/drivers/gpu/nova-core/fsp.rs b/drivers/gpu/nova-core/fsp.rs
index ab685fb4168f..961e96fe9484 100644
--- a/drivers/gpu/nova-core/fsp.rs
+++ b/drivers/gpu/nova-core/fsp.rs
@@ -267,7 +267,7 @@ fn frts_vidmem_offset(hal: &dyn hal::FspHal, fb_info: &FbSizes) -> Result<u64> {
/// Returns an in-place initializer for [`FspCotMessage`].
fn new<'a>(
fb_info: &FbSizes,
- fsp_fw: &'a FspFirmware,
+ fsp_fw: &'a FspFirmware<'_>,
args: &'a FmcBootArgs<'_>,
) -> Result<impl Init<Self> + 'a> {
let hal = hal::fsp_hal(args.chipset).ok_or(ENOTSUPP)?;
@@ -345,28 +345,28 @@ impl MessageToFsp for FspPrcMessage {
/// Bundled arguments for FMC boot via FSP Chain of Trust.
pub(crate) struct FmcBootArgs<'a> {
chipset: Chipset,
- fmc_boot_params: Coherent<GspFmcBootParams>,
+ fmc_boot_params: Coherent<'a, GspFmcBootParams>,
resume: bool,
// Additional dependencies required to be kept alive for FMC boot.
- _wpr_meta: Coherent<GspFwWprMeta>,
- _libos: &'a Coherent<[LibosMemoryRegionInitArgument]>,
+ _wpr_meta: Coherent<'a, GspFwWprMeta>,
+ _libos: &'a Coherent<'a, [LibosMemoryRegionInitArgument]>,
}
impl<'a> FmcBootArgs<'a> {
/// Builds FMC boot arguments, allocating the DMA-coherent boot parameter
/// structure that FSP will read.
pub(crate) fn new(
- dev: &device::Device<device::Bound>,
+ dev: &'a device::Device<device::Bound>,
chipset: Chipset,
- wpr_meta: Coherent<GspFwWprMeta>,
- libos: &'a Coherent<[LibosMemoryRegionInitArgument]>,
+ wpr_meta: Coherent<'a, GspFwWprMeta>,
+ libos: &'a Coherent<'a, [LibosMemoryRegionInitArgument]>,
resume: bool,
) -> Result<Self> {
let init = GspFmcBootParams::new(wpr_meta.dma_address(), libos.dma_address());
Ok(Self {
chipset,
- fmc_boot_params: Coherent::<GspFmcBootParams>::init(dev, GFP_KERNEL, init)?,
+ fmc_boot_params: Coherent::init(dev, GFP_KERNEL, init)?,
resume,
_wpr_meta: wpr_meta,
_libos: libos,
@@ -374,7 +374,7 @@ pub(crate) fn new(
}
/// Returns the FMC boot parameters allocation.
- pub(crate) fn boot_params(&self) -> &Coherent<GspFmcBootParams> {
+ pub(crate) fn boot_params(&self) -> &Coherent<'_, GspFmcBootParams> {
&self.fmc_boot_params
}
}
@@ -386,7 +386,7 @@ pub(crate) fn boot_params(&self) -> &Coherent<GspFmcBootParams> {
/// Chain of Trust boot.
pub(crate) struct Fsp<'a> {
falcon: Falcon<'a, FspEngine>,
- fsp_fw: FspFirmware,
+ fsp_fw: FspFirmware<'a>,
}
impl<'a> Fsp<'a> {
diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index fd1414004dd0..6de75d16488d 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -272,9 +272,9 @@ struct GspResources<'gpu> {
vgpu: VgpuManager,
/// GSP runtime data.
#[pin]
- gsp: Gsp,
+ gsp: Gsp<'gpu>,
/// GSP unload firmware bundle, if any.
- unload_bundle: Option<gsp::UnloadBundle>,
+ unload_bundle: Option<gsp::UnloadBundle<'gpu>>,
}
/// Structure holding the resources required to operate the GPU.
diff --git a/drivers/gpu/nova-core/gsp.rs b/drivers/gpu/nova-core/gsp.rs
index 13f361406a6c..25ea43f1cbe9 100644
--- a/drivers/gpu/nova-core/gsp.rs
+++ b/drivers/gpu/nova-core/gsp.rs
@@ -115,11 +115,11 @@ fn init(view: CoherentView<'_, Self>, start: DmaAddress) -> Result<()> {
/// then pp points to index into the buffer where the next logging entry will
/// be written. Therefore, the logging data is valid if:
/// 1 <= pp < sizeof(buffer)/sizeof(u64)
-struct LogBuffer(Coherent<[u8; LOG_BUFFER_SIZE]>);
+struct LogBuffer<'a>(Coherent<'a, [u8; LOG_BUFFER_SIZE]>);
-impl LogBuffer {
+impl<'a> LogBuffer<'a> {
/// Creates a new `LogBuffer` mapped on `dev`.
- fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
+ fn new(dev: &'a device::Device<device::Bound>) -> Result<Self> {
let obj = Self(Coherent::zeroed(dev, GFP_KERNEL)?);
let start_addr = obj.0.dma_address();
@@ -135,33 +135,33 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
}
}
-struct LogBuffers {
+struct LogBuffers<'a> {
/// Init log buffer.
- loginit: LogBuffer,
+ loginit: LogBuffer<'a>,
/// Interrupts log buffer.
- logintr: LogBuffer,
+ logintr: LogBuffer<'a>,
/// RM log buffer.
- logrm: LogBuffer,
+ logrm: LogBuffer<'a>,
}
/// GSP runtime data.
#[pin_data]
-pub(crate) struct Gsp {
+pub(crate) struct Gsp<'gsp> {
/// Libos arguments.
- pub(crate) libos: Coherent<[LibosMemoryRegionInitArgument]>,
+ pub(crate) libos: Coherent<'gsp, [LibosMemoryRegionInitArgument]>,
/// Log buffers, optionally exposed via debugfs.
#[pin]
- logs: debugfs::Scope<LogBuffers>,
+ logs: debugfs::Scope<LogBuffers<'gsp>>,
/// Command queue.
#[pin]
- pub(crate) cmdq: Cmdq,
+ pub(crate) cmdq: Cmdq<'gsp>,
/// RM arguments.
- rmargs: Coherent<GspArgumentsPadded>,
+ rmargs: Coherent<'gsp, GspArgumentsPadded>,
}
-impl Gsp {
+impl<'gsp> Gsp<'gsp> {
// Creates an in-place initializer for a `Gsp` manager for `pdev`.
- pub(crate) fn new(pdev: &pci::Device<device::Bound>) -> impl PinInit<Self, Error> + '_ {
+ pub(crate) fn new(pdev: &'gsp pci::Device<device::Bound>) -> impl PinInit<Self, Error> + 'gsp {
pin_init::pin_init_scope(move || {
let dev = pdev.as_ref();
@@ -223,4 +223,4 @@ pub(crate) fn get_static_info(&self, bar: Bar0<'_>) -> Result<commands::GetGspSt
}
/// Opaque bundle required to unload the GSP. Created by [`Gsp::boot`], consumed by [`Gsp::unload`].
-pub(crate) struct UnloadBundle(KBox<dyn hal::UnloadBundle>);
+pub(crate) struct UnloadBundle<'a>(KBox<dyn hal::UnloadBundle + 'a>);
diff --git a/drivers/gpu/nova-core/gsp/boot.rs b/drivers/gpu/nova-core/gsp/boot.rs
index e03700ee7bea..e32c9e1f33ab 100644
--- a/drivers/gpu/nova-core/gsp/boot.rs
+++ b/drivers/gpu/nova-core/gsp/boot.rs
@@ -22,7 +22,7 @@
},
};
-impl super::Gsp {
+impl<'gsp> super::Gsp<'gsp> {
/// Attempt to boot the GSP.
///
/// This is a GPU-dependent and complex procedure that involves loading firmware files from
@@ -33,8 +33,8 @@ impl super::Gsp {
/// [`Self::unload`]) returned.
pub(crate) fn boot(
self: Pin<&mut Self>,
- mut ctx: super::GspBootContext<'_, '_>,
- ) -> Result<Option<super::UnloadBundle>> {
+ mut ctx: super::GspBootContext<'_, 'gsp>,
+ ) -> Result<Option<super::UnloadBundle<'gsp>>> {
let pdev = ctx.pdev;
let bar = ctx.bar;
let chipset = ctx.chipset;
@@ -88,7 +88,7 @@ pub(crate) fn boot(
/// Shut down the GSP and wait until it is offline.
fn shutdown_gsp(
- cmdq: &Cmdq,
+ cmdq: &Cmdq<'_>,
bar: Bar0<'_>,
gsp_falcon: &Falcon<'_, Gsp>,
mode: commands::PowerStateLevel,
@@ -113,7 +113,7 @@ fn shutdown_gsp(
pub(crate) fn unload(
&self,
mut ctx: super::GspBootContext<'_, '_>,
- unload_bundle: Option<super::UnloadBundle>,
+ unload_bundle: Option<super::UnloadBundle<'_>>,
) -> Result {
let dev = ctx.dev();
diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index 6da728201281..fe087ce315b0 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -25,10 +25,7 @@
new_mutex,
prelude::*,
ptr,
- sync::{
- aref::ARef,
- Mutex, //
- },
+ sync::Mutex,
time::Delta,
transmute::{
AsBytes,
@@ -230,19 +227,19 @@ unsafe impl FromBytes for GspMem {}
/// pointer and the GSP read pointer. This region is returned by [`Self::driver_write_area`].
/// * The driver owns (i.e. can read from) the part of the GSP message queue between the CPU read
/// pointer and the GSP write pointer. This region is returned by [`Self::driver_read_area`].
-struct DmaGspMem(Coherent<GspMem>);
+struct DmaGspMem<'a>(Coherent<'a, GspMem>);
-impl DmaGspMem {
+impl<'a> DmaGspMem<'a> {
/// Allocate a new instance and map it for `dev`.
- fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
+ fn new(dev: &'a device::Device<device::Bound>) -> Result<Self> {
const MSGQ_SIZE: u32 = num::usize_into_u32::<{ size_of::<Msgq>() }>();
const RX_HDR_OFF: u32 = num::usize_into_u32::<{ mem::offset_of!(Msgq, rx) }>();
- let mut gsp_mem = CoherentBox::<GspMem>::zeroed(dev, GFP_KERNEL)?;
+ let mut gsp_mem = CoherentBox::<'_, GspMem>::zeroed(dev, GFP_KERNEL)?;
gsp_mem.cpuq.tx = MsgqTxHeader::new(MSGQ_SIZE, RX_HDR_OFF, MSGQ_NUM_PAGES);
gsp_mem.cpuq.rx = MsgqRxHeader::new();
- let gsp_mem: Coherent<_> = gsp_mem.into();
+ let gsp_mem: Coherent<'_, _> = gsp_mem.into();
PteArray::init(io_project!(gsp_mem, .ptes), gsp_mem.dma_address())?;
Ok(Self(gsp_mem))
@@ -483,15 +480,15 @@ struct GspMessage<'a> {
/// Provides the ability to send commands and receive messages from the GSP using a shared memory
/// area.
#[pin_data]
-pub(crate) struct Cmdq {
+pub(crate) struct Cmdq<'cmdq> {
/// Inner mutex-protected state.
#[pin]
- inner: Mutex<CmdqInner>,
+ inner: Mutex<CmdqInner<'cmdq>>,
/// DMA address of the command queue's shared memory region.
pub(super) dma_addr: DmaAddress,
}
-impl Cmdq {
+impl<'cmdq> Cmdq<'cmdq> {
/// Offset of the data after the PTEs.
const POST_PTE_OFFSET: usize = core::mem::offset_of!(GspMem, cpuq);
@@ -512,14 +509,16 @@ impl Cmdq {
pub(super) const RECEIVE_TIMEOUT: Delta = Delta::from_secs(5);
/// Creates a new command queue for `dev`.
- pub(crate) fn new(dev: &device::Device<device::Bound>) -> impl PinInit<Self, Error> + '_ {
+ pub(crate) fn new(
+ dev: &'cmdq device::Device<device::Bound>,
+ ) -> impl PinInit<Self, Error> + 'cmdq {
pin_init_scope(move || {
let gsp_mem = DmaGspMem::new(dev)?;
Ok(try_pin_init!(Self {
dma_addr: gsp_mem.0.dma_address(),
inner <- new_mutex!(CmdqInner {
- dev: dev.into(),
+ dev,
gsp_mem,
seq: 0,
}),
@@ -610,16 +609,16 @@ pub(crate) fn receive_msg<M: MessageFromGsp>(&self, timeout: Delta) -> Result<M>
}
/// Inner mutex protected state of [`Cmdq`].
-struct CmdqInner {
+struct CmdqInner<'a> {
/// Device this command queue belongs to.
- dev: ARef<device::Device>,
+ dev: &'a device::Device,
/// Current command sequence number.
seq: u32,
/// Memory area shared with the GSP for communicating commands and messages.
- gsp_mem: DmaGspMem,
+ gsp_mem: DmaGspMem<'a>,
}
-impl CmdqInner {
+impl CmdqInner<'_> {
/// Timeout for waiting for space on the command queue.
const ALLOCATE_TIMEOUT: Delta = Delta::from_secs(1);
diff --git a/drivers/gpu/nova-core/gsp/commands.rs b/drivers/gpu/nova-core/gsp/commands.rs
index ffc25fd8c47b..69d7d41c1791 100644
--- a/drivers/gpu/nova-core/gsp/commands.rs
+++ b/drivers/gpu/nova-core/gsp/commands.rs
@@ -187,7 +187,7 @@ fn read(
}
/// Waits for GSP initialization to complete.
-pub(crate) fn wait_gsp_init_done(cmdq: &Cmdq) -> Result {
+pub(crate) fn wait_gsp_init_done(cmdq: &Cmdq<'_>) -> Result {
loop {
match cmdq.receive_msg::<GspInitDone>(Cmdq::RECEIVE_TIMEOUT) {
Ok(_) => break Ok(()),
diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
index 05f54fee6186..8778c4bf79c0 100644
--- a/drivers/gpu/nova-core/gsp/fw.rs
+++ b/drivers/gpu/nova-core/gsp/fw.rs
@@ -179,7 +179,7 @@ impl GspFwWprMeta {
/// Returns an initializer for a `GspFwWprMeta` suitable for booting `gsp_firmware` using the
/// framebuffer ranges `ranges`.
pub(crate) fn from_ranges<'a>(
- gsp_firmware: &'a GspFirmware,
+ gsp_firmware: &'a GspFirmware<'_>,
ranges: &'a FbRanges,
) -> impl Init<Self> + 'a {
let init_inner = init!(bindings::GspFwWprMeta {
@@ -231,7 +231,7 @@ pub(crate) fn from_ranges<'a>(
///
/// The region offsets are left at zero: the ACR ucode computes them when it sets up WPR2.
pub(crate) fn from_sizes<'a>(
- gsp_firmware: &'a GspFirmware,
+ gsp_firmware: &'a GspFirmware<'_>,
sizes: &'a FbSizes,
) -> impl Init<Self> + 'a {
/// VGA workspace size to reserve at the end of the framebuffer, in bytes.
@@ -665,7 +665,7 @@ unsafe impl FromBytes for LibosMemoryRegionInitArgument {}
impl LibosMemoryRegionInitArgument {
pub(crate) fn new<'a, A: AsBytes + FromBytes + KnownSize + ?Sized>(
name: &'static str,
- obj: &'a Coherent<A>,
+ obj: &'a Coherent<'_, A>,
) -> impl Init<Self> + 'a {
/// Generates the `ID8` identifier required for some GSP objects.
fn id8(name: &str) -> u64 {
@@ -897,7 +897,7 @@ pub(crate) struct GspArgumentsCached {
impl GspArgumentsCached {
/// Creates the arguments for starting the GSP up using `cmdq` as its command queue.
- pub(crate) fn new(cmdq: &Cmdq) -> impl Init<Self> + '_ {
+ pub(crate) fn new<'a, 'b>(cmdq: &'a Cmdq<'b>) -> impl Init<Self> + use<'a, 'b> {
let init_inner = init!(bindings::GSP_ARGUMENTS_CACHED {
messageQueueInitArguments <- MessageQueueInitArguments::new(cmdq),
bDmemStack: 1,
@@ -924,7 +924,7 @@ pub(crate) struct GspArgumentsPadded {
}
impl GspArgumentsPadded {
- pub(crate) fn new(cmdq: &Cmdq) -> impl Init<Self> + '_ {
+ pub(crate) fn new<'a, 'b>(cmdq: &'a Cmdq<'b>) -> impl Init<Self> + use<'a, 'b> {
init!(GspArgumentsPadded {
inner <- GspArgumentsCached::new(cmdq),
..Zeroable::init_zeroed()
@@ -944,7 +944,7 @@ unsafe impl FromBytes for GspArgumentsPadded {}
impl MessageQueueInitArguments {
/// Creates a new init arguments structure for `cmdq`.
- fn new(cmdq: &Cmdq) -> impl Init<Self> + '_ {
+ fn new<'a, 'b>(cmdq: &'a Cmdq<'b>) -> impl Init<Self> + use<'a, 'b> {
init!(MessageQueueInitArguments {
sharedMemPhysAddr: cmdq.dma_addr,
pageTableEntryCount: num::usize_into_u32::<{ Cmdq::NUM_PTES }>(),
diff --git a/drivers/gpu/nova-core/gsp/hal.rs b/drivers/gpu/nova-core/gsp/hal.rs
index 5850fa0fe0e9..d8329f6fcc65 100644
--- a/drivers/gpu/nova-core/gsp/hal.rs
+++ b/drivers/gpu/nova-core/gsp/hal.rs
@@ -35,12 +35,12 @@ pub(super) trait GspHal: Send {
///
/// Upon success, returns the [`crate::gsp::UnloadBundle`] to use with [`Gsp::unload`], if one
/// could be created.
- fn boot(
+ fn boot<'gpu>(
&self,
- gsp: &Gsp,
- ctx: &mut GspBootContext<'_, '_>,
- gsp_fw: &GspFirmware,
- ) -> Result<Option<crate::gsp::UnloadBundle>>;
+ gsp: &Gsp<'gpu>,
+ ctx: &mut GspBootContext<'_, 'gpu>,
+ gsp_fw: &GspFirmware<'gpu>,
+ ) -> Result<Option<super::UnloadBundle<'gpu>>>;
/// Performs HAL-specific post-GSP boot tasks.
///
@@ -48,9 +48,9 @@ fn boot(
/// after the initialization commands have been pushed onto its queue.
fn post_boot(
&self,
- _gsp: &Gsp,
+ _gsp: &Gsp<'_>,
_ctx: &mut GspBootContext<'_, '_>,
- _gsp_fw: &GspFirmware,
+ _gsp_fw: &GspFirmware<'_>,
) -> Result {
Ok(())
}
diff --git a/drivers/gpu/nova-core/gsp/hal/gh100.rs b/drivers/gpu/nova-core/gsp/hal/gh100.rs
index e283429a95dd..91201b51030e 100644
--- a/drivers/gpu/nova-core/gsp/hal/gh100.rs
+++ b/drivers/gpu/nova-core/gsp/hal/gh100.rs
@@ -58,7 +58,7 @@ fn combined_addr(&self) -> u64 {
fn lockdown_released_or_error(
&self,
gsp_falcon: &Falcon<'_, GspEngine>,
- fmc_boot_params: &Coherent<GspFmcBootParams>,
+ fmc_boot_params: &Coherent<'_, GspFmcBootParams>,
) -> bool {
// GSP-FMC normally clears the boot parameters address from the mailboxes early during
// boot. If the address is still there, keep polling rather than treating it as an error.
@@ -75,7 +75,7 @@ fn lockdown_released_or_error(
fn wait_for_gsp_lockdown_release(
dev: &device::Device<device::Bound>,
gsp_falcon: &Falcon<'_, GspEngine>,
- fmc_boot_params: &Coherent<GspFmcBootParams>,
+ fmc_boot_params: &Coherent<'_, GspFmcBootParams>,
) -> Result {
dev_dbg!(dev, "Waiting for GSP lockdown release\n");
@@ -141,12 +141,12 @@ impl GspHal for Gh100 {
///
/// This path uses FSP to establish a chain of trust and boot GSP-FMC. FSP handles
/// the GSP boot internally - no manual GSP reset/boot is needed.
- fn boot(
+ fn boot<'gpu>(
&self,
- gsp: &Gsp,
- ctx: &mut GspBootContext<'_, '_>,
- gsp_fw: &GspFirmware,
- ) -> Result<Option<crate::gsp::UnloadBundle>> {
+ gsp: &Gsp<'gpu>,
+ ctx: &mut GspBootContext<'_, 'gpu>,
+ gsp_fw: &GspFirmware<'gpu>,
+ ) -> Result<Option<crate::gsp::UnloadBundle<'gpu>>> {
let dev = ctx.dev();
let chipset = ctx.chipset;
let gsp_falcon = ctx.gsp_falcon;
@@ -159,7 +159,7 @@ fn boot(
let args = FmcBootArgs::new(dev, chipset, wpr_meta, &gsp.libos, false)?;
let unload_bundle = crate::gsp::UnloadBundle(
- KBox::new(FspUnloadBundle, GFP_KERNEL)? as KBox<dyn UnloadBundle>
+ KBox::new(FspUnloadBundle, GFP_KERNEL)? as KBox<dyn UnloadBundle + 'gpu>
);
// Wait for the GSP RISC-V core to halt in case of error. We create this guard after `args`
diff --git a/drivers/gpu/nova-core/gsp/hal/tu102.rs b/drivers/gpu/nova-core/gsp/hal/tu102.rs
index a5c0ca355493..f315013cff86 100644
--- a/drivers/gpu/nova-core/gsp/hal/tu102.rs
+++ b/drivers/gpu/nova-core/gsp/hal/tu102.rs
@@ -52,12 +52,12 @@
//
// Since there are two variants of the prepared firmware (with and without a bootloader), this type
// abstracts the difference.
-enum FwsecUnloadFirmware {
+enum FwsecUnloadFirmware<'a> {
WithoutBl(FwsecFirmware),
- WithBl(FwsecFirmwareWithBl),
+ WithBl(FwsecFirmwareWithBl<'a>),
}
-impl FwsecUnloadFirmware {
+impl FwsecUnloadFirmware<'_> {
/// Runs the FWSEC SB firmware.
fn run(
&self,
@@ -74,12 +74,12 @@ fn run(
// Contains the firmware required to fully reset GSP on chipsets where the GSP is started using
// FWSEC/Booter.
-struct Sec2UnloadBundle {
- fwsec_sb: FwsecUnloadFirmware,
+struct Sec2UnloadBundle<'a> {
+ fwsec_sb: FwsecUnloadFirmware<'a>,
booter_unloader: BooterFirmware,
}
-impl UnloadBundle for Sec2UnloadBundle {
+impl UnloadBundle for Sec2UnloadBundle<'_> {
fn run(&self, ctx: &mut GspBootContext<'_, '_>) -> Result {
let dev = ctx.dev();
let bar = ctx.bar;
@@ -213,14 +213,14 @@ fn run_fwsec_frts(
}
/// Load and prepare the resources required to properly reset the GSP after it has been stopped.
- fn build_unload_bundle(
+ fn build_unload_bundle<'gpu>(
&self,
- dev: &device::Device<device::Bound>,
+ dev: &'gpu device::Device<device::Bound>,
chipset: Chipset,
bios: &Vbios,
gsp_falcon: &Falcon<'_, GspEngine>,
sec2_falcon: &Falcon<'_, Sec2>,
- ) -> Result<crate::gsp::UnloadBundle> {
+ ) -> Result<crate::gsp::UnloadBundle<'gpu>> {
// Load the FWSEC SB firmware, as well as its bootloader if required.
let fwsec_sb = FwsecFirmware::new(dev, gsp_falcon, bios, FwsecCommand::Sb)?;
let fwsec_sb = if self.needs_fwsec_bootloader {
@@ -241,18 +241,18 @@ fn build_unload_bundle(
},
GFP_KERNEL,
)
- .map(|b| crate::gsp::UnloadBundle(b))
+ .map(|b| crate::gsp::UnloadBundle(b as KBox<dyn UnloadBundle + 'gpu>))
.map_err(Into::into)
}
}
impl GspHal for Tu102 {
- fn boot(
+ fn boot<'gpu>(
&self,
- gsp: &Gsp,
- ctx: &mut GspBootContext<'_, '_>,
- gsp_fw: &GspFirmware,
- ) -> Result<Option<crate::gsp::UnloadBundle>> {
+ gsp: &Gsp<'gpu>,
+ ctx: &mut GspBootContext<'_, 'gpu>,
+ gsp_fw: &GspFirmware<'gpu>,
+ ) -> Result<Option<crate::gsp::UnloadBundle<'gpu>>> {
let dev = ctx.dev();
let bar = ctx.bar;
let chipset = ctx.chipset;
@@ -317,9 +317,9 @@ fn boot(
fn post_boot(
&self,
- gsp: &Gsp,
+ gsp: &Gsp<'_>,
ctx: &mut GspBootContext<'_, '_>,
- gsp_fw: &GspFirmware,
+ gsp_fw: &GspFirmware<'_>,
) -> Result {
GspSequencer::run(&gsp.cmdq, ctx, &gsp.libos, gsp_fw.bootloader.app_version)?;
diff --git a/drivers/gpu/nova-core/gsp/sequencer.rs b/drivers/gpu/nova-core/gsp/sequencer.rs
index bcad1421953a..dae34c11eb05 100644
--- a/drivers/gpu/nova-core/gsp/sequencer.rs
+++ b/drivers/gpu/nova-core/gsp/sequencer.rs
@@ -138,7 +138,7 @@ pub(crate) struct GspSequencer<'a> {
/// GSP falcon for core operations.
gsp_falcon: &'a Falcon<'a, Gsp>,
/// LibOS memory region init arguments.
- libos: &'a Coherent<[LibosMemoryRegionInitArgument]>,
+ libos: &'a Coherent<'a, [LibosMemoryRegionInitArgument]>,
/// Bootloader application version.
bootloader_app_version: u32,
/// Device for logging.
@@ -338,9 +338,9 @@ fn next(&mut self) -> Option<Self::Item> {
impl<'a> GspSequencer<'a> {
pub(crate) fn run(
- cmdq: &Cmdq,
+ cmdq: &Cmdq<'_>,
ctx: &'a GspBootContext<'_, '_>,
- libos: &'a Coherent<[LibosMemoryRegionInitArgument]>,
+ libos: &'a Coherent<'a, [LibosMemoryRegionInitArgument]>,
bootloader_app_version: u32,
) -> Result {
let seq_info = loop {
diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
index 79f453e9ec0b..4ce914b7d1da 100644
--- a/rust/kernel/dma.rs
+++ b/rust/kernel/dma.rs
@@ -24,7 +24,6 @@
},
prelude::*,
ptr::KnownSize,
- sync::aref::ARef,
transmute::{
AsBytes,
FromBytes, //
@@ -223,7 +222,7 @@ pub const fn value(&self) -> u64 {
///
/// # fn test(dev: &Device<Bound>) -> Result {
/// let attribs = DMA_ATTR_FORCE_CONTIGUOUS | DMA_ATTR_NO_WARN;
-/// let c: Coherent<[u64]> =
+/// let c: Coherent<'_, [u64]> =
/// Coherent::zeroed_slice_with_attrs(dev, 4, GFP_KERNEL, attribs)?;
/// # Ok::<(), Error>(()) }
/// ```
@@ -390,9 +389,9 @@ fn from(direction: DataDirection) -> Self {
/// };
///
/// # fn test(dev: &Device<Bound>) -> Result {
-/// let mut dmem: CoherentBox<u64> = CoherentBox::zeroed(dev, GFP_KERNEL)?;
+/// let mut dmem: CoherentBox<'_, u64> = CoherentBox::zeroed(dev, GFP_KERNEL)?;
/// *dmem = 42;
-/// let dmem: Coherent<u64> = dmem.into();
+/// let dmem: Coherent<'_, u64> = dmem.into();
/// # Ok::<(), Error>(()) }
/// ```
///
@@ -410,18 +409,18 @@ fn from(direction: DataDirection) -> Self {
/// };
///
/// # fn test(dev: &Device<Bound>) -> Result {
-/// let mut dmem: CoherentBox<[u64]> = CoherentBox::zeroed_slice(dev, 4, GFP_KERNEL)?;
+/// let mut dmem: CoherentBox<'_, [u64]> = CoherentBox::zeroed_slice(dev, 4, GFP_KERNEL)?;
/// dmem.fill(42);
-/// let dmem: Coherent<[u64]> = dmem.into();
+/// let dmem: Coherent<'_, [u64]> = dmem.into();
/// # Ok::<(), Error>(()) }
/// ```
-pub struct CoherentBox<T: KnownSize + ?Sized>(Coherent<T>);
+pub struct CoherentBox<'a, T: KnownSize + ?Sized>(Coherent<'a, T>);
-impl<T: AsBytes + FromBytes> CoherentBox<[T]> {
+impl<'a, T: AsBytes + FromBytes> CoherentBox<'a, [T]> {
/// [`CoherentBox`] variant of [`Coherent::zeroed_slice_with_attrs`].
#[inline]
pub fn zeroed_slice_with_attrs(
- dev: &device::Device<Bound>,
+ dev: &'a device::Device<Bound>,
count: usize,
gfp_flags: kernel::alloc::Flags,
dma_attrs: Attrs,
@@ -432,7 +431,7 @@ pub fn zeroed_slice_with_attrs(
/// Same as [CoherentBox::zeroed_slice_with_attrs], but with `dma::Attrs(0)`.
#[inline]
pub fn zeroed_slice(
- dev: &device::Device<Bound>,
+ dev: &'a device::Device<Bound>,
count: usize,
gfp_flags: kernel::alloc::Flags,
) -> Result<Self> {
@@ -480,14 +479,14 @@ pub fn init_at<E>(&mut self, i: usize, init: impl Init<T, E>) -> Result
///
/// # fn test(dev: &Device<Bound>) -> Result {
/// let data = [0u8, 1u8, 2u8, 3u8];
- /// let c: CoherentBox<[u8]> =
+ /// let c: CoherentBox<'_, [u8]> =
/// CoherentBox::from_slice_with_attrs(dev, &data, GFP_KERNEL, DMA_ATTR_NO_WARN)?;
///
/// assert_eq!(c.deref(), &data);
/// # Ok::<(), Error>(()) }
/// ```
pub fn from_slice_with_attrs(
- dev: &device::Device<Bound>,
+ dev: &'a device::Device<Bound>,
data: &[T],
gfp_flags: kernel::alloc::Flags,
dma_attrs: Attrs,
@@ -512,7 +511,7 @@ pub fn from_slice_with_attrs(
/// `dma_attrs` is 0 by default.
#[inline]
pub fn from_slice(
- dev: &device::Device<Bound>,
+ dev: &'a device::Device<Bound>,
data: &[T],
gfp_flags: kernel::alloc::Flags,
) -> Result<Self>
@@ -523,11 +522,11 @@ pub fn from_slice(
}
}
-impl<T: AsBytes + FromBytes> CoherentBox<T> {
+impl<'a, T: AsBytes + FromBytes> CoherentBox<'a, T> {
/// Same as [`CoherentBox::zeroed_slice_with_attrs`], but for a single element.
#[inline]
pub fn zeroed_with_attrs(
- dev: &device::Device<Bound>,
+ dev: &'a device::Device<Bound>,
gfp_flags: kernel::alloc::Flags,
dma_attrs: Attrs,
) -> Result<Self> {
@@ -536,12 +535,12 @@ pub fn zeroed_with_attrs(
/// Same as [`CoherentBox::zeroed_slice`], but for a single element.
#[inline]
- pub fn zeroed(dev: &device::Device<Bound>, gfp_flags: kernel::alloc::Flags) -> Result<Self> {
+ pub fn zeroed(dev: &'a device::Device<Bound>, gfp_flags: kernel::alloc::Flags) -> Result<Self> {
Self::zeroed_with_attrs(dev, gfp_flags, Attrs(0))
}
}
-impl<T: KnownSize + ?Sized> Deref for CoherentBox<T> {
+impl<T: KnownSize + ?Sized> Deref for CoherentBox<'_, T> {
type Target = T;
#[inline]
@@ -554,7 +553,7 @@ fn deref(&self) -> &Self::Target {
}
}
-impl<T: AsBytes + FromBytes + KnownSize + ?Sized> DerefMut for CoherentBox<T> {
+impl<T: AsBytes + FromBytes + KnownSize + ?Sized> DerefMut for CoherentBox<'_, T> {
#[inline]
fn deref_mut(&mut self) -> &mut Self::Target {
// SAFETY:
@@ -565,9 +564,9 @@ fn deref_mut(&mut self) -> &mut Self::Target {
}
}
-impl<T: AsBytes + FromBytes + KnownSize + ?Sized> From<CoherentBox<T>> for Coherent<T> {
+impl<'a, T: AsBytes + FromBytes + KnownSize + ?Sized> From<CoherentBox<'a, T>> for Coherent<'a, T> {
#[inline]
- fn from(value: CoherentBox<T>) -> Self {
+ fn from(value: CoherentBox<'a, T>) -> Self {
value.0
}
}
@@ -588,26 +587,20 @@ fn from(value: CoherentBox<T>) -> Self {
/// to an allocated region of coherent memory and `dma_addr` is the DMA address base of the
/// region.
/// - The size in bytes of the allocation is equal to size information via pointer.
-// TODO
//
-// DMA allocations potentially carry device resources (e.g.IOMMU mappings), hence for soundness
-// reasons DMA allocation would need to be embedded in a `Devres` container, in order to ensure
-// that device resources can never survive device unbind.
-//
-// However, it is neither desirable nor necessary to protect the allocated memory of the DMA
-// allocation from surviving device unbind; it would require RCU read side critical sections to
-// access the memory, which may require subsequent unnecessary copies.
-//
-// Hence, find a way to revoke the device resources of a `Coherent`, but not the
-// entire `Coherent` including the allocated memory itself.
-pub struct Coherent<T: KnownSize + ?Sized> {
- dev: ARef<device::Device>,
+// The lifetime parameter ties DMA allocations to the device's bound scope, ensuring they are freed
+// before the device is unbound under normal circumstances. However, if a `Coherent` is leaked (e.g.
+// via `mem::forget`), device resources such as IOMMU mappings will not be released. Making all
+// constructors `unsafe` to prevent this is considered too restrictive for the common case; this
+// soundness hole is accepted for now.
+pub struct Coherent<'a, T: KnownSize + ?Sized> {
+ dev: &'a device::Device<Bound>,
dma_addr: DmaAddress,
cpu_addr: NonNull<T>,
dma_attrs: Attrs,
}
-impl<T: KnownSize + ?Sized> Coherent<T> {
+impl<T: KnownSize + ?Sized> Coherent<'_, T> {
/// Returns the size in bytes of this allocation.
#[inline]
pub fn size(&self) -> usize {
@@ -663,10 +656,10 @@ pub unsafe fn as_mut(&self) -> &mut T {
}
}
-impl<T: AsBytes + FromBytes> Coherent<T> {
+impl<'a, T: AsBytes + FromBytes> Coherent<'a, T> {
/// Allocates a region of `T` of coherent memory.
fn alloc_with_attrs(
- dev: &device::Device<Bound>,
+ dev: &'a device::Device<Bound>,
gfp_flags: kernel::alloc::Flags,
dma_attrs: Attrs,
) -> Result<Self> {
@@ -692,9 +685,9 @@ fn alloc_with_attrs(
// INVARIANT:
// - We just successfully allocated a coherent region which is adequately sized for `T`,
// hence the cpu address is valid.
- // - We also hold a refcounted reference to the device.
+ // - `dev` is a valid reference to a bound device that outlives this allocation.
Ok(Self {
- dev: dev.into(),
+ dev,
dma_addr,
cpu_addr,
dma_attrs,
@@ -716,13 +709,13 @@ fn alloc_with_attrs(
/// };
///
/// # fn test(dev: &Device<Bound>) -> Result {
- /// let c: Coherent<[u64; 4]> =
+ /// let c: Coherent<'_, [u64; 4]> =
/// Coherent::zeroed_with_attrs(dev, GFP_KERNEL, DMA_ATTR_NO_WARN)?;
/// # Ok::<(), Error>(()) }
/// ```
#[inline]
pub fn zeroed_with_attrs(
- dev: &device::Device<Bound>,
+ dev: &'a device::Device<Bound>,
gfp_flags: kernel::alloc::Flags,
dma_attrs: Attrs,
) -> Result<Self> {
@@ -732,14 +725,14 @@ pub fn zeroed_with_attrs(
/// Performs the same functionality as [`Coherent::zeroed_with_attrs`], except the
/// `dma_attrs` is 0 by default.
#[inline]
- pub fn zeroed(dev: &device::Device<Bound>, gfp_flags: kernel::alloc::Flags) -> Result<Self> {
+ pub fn zeroed(dev: &'a device::Device<Bound>, gfp_flags: kernel::alloc::Flags) -> Result<Self> {
Self::zeroed_with_attrs(dev, gfp_flags, Attrs(0))
}
/// Same as [`Coherent::zeroed_with_attrs`], but instead of a zero-initialization the memory is
/// initialized with `init`.
pub fn init_with_attrs<E>(
- dev: &device::Device<Bound>,
+ dev: &'a device::Device<Bound>,
gfp_flags: kernel::alloc::Flags,
dma_attrs: Attrs,
init: impl Init<T, E>,
@@ -764,7 +757,7 @@ pub fn init_with_attrs<E>(
/// with `init`.
#[inline]
pub fn init<E>(
- dev: &device::Device<Bound>,
+ dev: &'a device::Device<Bound>,
gfp_flags: kernel::alloc::Flags,
init: impl Init<T, E>,
) -> Result<Self>
@@ -776,11 +769,11 @@ pub fn init<E>(
/// Allocates a region of `[T; len]` of coherent memory.
fn alloc_slice_with_attrs(
- dev: &device::Device<Bound>,
+ dev: &'a device::Device<Bound>,
len: usize,
gfp_flags: kernel::alloc::Flags,
dma_attrs: Attrs,
- ) -> Result<Coherent<[T]>> {
+ ) -> Result<Coherent<'a, [T]>> {
const {
assert!(
core::mem::size_of::<T>() > 0,
@@ -809,9 +802,9 @@ fn alloc_slice_with_attrs(
// INVARIANT:
// - We just successfully allocated a coherent region which is adequately sized for
// `[T; len]`, hence the cpu address is valid.
- // - We also hold a refcounted reference to the device.
+ // - `dev` is a valid reference to a bound device that outlives this allocation.
Ok(Coherent {
- dev: dev.into(),
+ dev,
dma_addr,
cpu_addr,
dma_attrs,
@@ -836,17 +829,17 @@ fn alloc_slice_with_attrs(
/// };
///
/// # fn test(dev: &Device<Bound>) -> Result {
- /// let c: Coherent<[u64]> =
+ /// let c: Coherent<'_, [u64]> =
/// Coherent::zeroed_slice_with_attrs(dev, 4, GFP_KERNEL, DMA_ATTR_NO_WARN)?;
/// # Ok::<(), Error>(()) }
/// ```
#[inline]
pub fn zeroed_slice_with_attrs(
- dev: &device::Device<Bound>,
+ dev: &'a device::Device<Bound>,
len: usize,
gfp_flags: kernel::alloc::Flags,
dma_attrs: Attrs,
- ) -> Result<Coherent<[T]>> {
+ ) -> Result<Coherent<'a, [T]>> {
Coherent::alloc_slice_with_attrs(dev, len, gfp_flags | __GFP_ZERO, dma_attrs)
}
@@ -854,10 +847,10 @@ pub fn zeroed_slice_with_attrs(
/// `dma_attrs` is 0 by default.
#[inline]
pub fn zeroed_slice(
- dev: &device::Device<Bound>,
+ dev: &'a device::Device<Bound>,
len: usize,
gfp_flags: kernel::alloc::Flags,
- ) -> Result<Coherent<[T]>> {
+ ) -> Result<Coherent<'a, [T]>> {
Self::zeroed_slice_with_attrs(dev, len, gfp_flags, Attrs(0))
}
@@ -876,18 +869,18 @@ pub fn zeroed_slice(
/// # fn test(dev: &Device<Bound>) -> Result {
/// let data = [0u8, 1u8, 2u8, 3u8];
/// // `c` has the same content as `data`.
- /// let c: Coherent<[u8]> =
+ /// let c: Coherent<'_, [u8]> =
/// Coherent::from_slice_with_attrs(dev, &data, GFP_KERNEL, DMA_ATTR_NO_WARN)?;
///
/// # Ok::<(), Error>(()) }
/// ```
#[inline]
pub fn from_slice_with_attrs(
- dev: &device::Device<Bound>,
+ dev: &'a device::Device<Bound>,
data: &[T],
gfp_flags: kernel::alloc::Flags,
dma_attrs: Attrs,
- ) -> Result<Coherent<[T]>>
+ ) -> Result<Coherent<'a, [T]>>
where
T: Copy,
{
@@ -898,10 +891,10 @@ pub fn from_slice_with_attrs(
/// `dma_attrs` is 0 by default.
#[inline]
pub fn from_slice(
- dev: &device::Device<Bound>,
+ dev: &'a device::Device<Bound>,
data: &[T],
gfp_flags: kernel::alloc::Flags,
- ) -> Result<Coherent<[T]>>
+ ) -> Result<Coherent<'a, [T]>>
where
T: Copy,
{
@@ -909,7 +902,7 @@ pub fn from_slice(
}
}
-impl<T> Coherent<[T]> {
+impl<T> Coherent<'_, [T]> {
/// Returns the number of elements `T` in this allocation.
///
/// Note that this is not the size of the allocation in bytes, which is provided by
@@ -922,10 +915,10 @@ pub fn len(&self) -> usize {
}
/// Note that the device configured to do DMA must be halted before this object is dropped.
-impl<T: KnownSize + ?Sized> Drop for Coherent<T> {
+impl<T: KnownSize + ?Sized> Drop for Coherent<'_, T> {
fn drop(&mut self) {
let size = T::size(self.cpu_addr.as_ptr());
- // SAFETY: Device pointer is guaranteed as valid by the type invariant on `Device`.
+ // SAFETY: Device pointer is guaranteed as valid by the lifetime of this `Coherent`.
// The cpu address, and the dma address are valid due to the type invariants on
// `Coherent`.
unsafe {
@@ -942,15 +935,15 @@ fn drop(&mut self) {
// SAFETY: It is safe to send a `Coherent` to another thread if `T`
// can be sent to another thread.
-unsafe impl<T: KnownSize + Send + ?Sized> Send for Coherent<T> {}
+unsafe impl<T: KnownSize + Send + ?Sized> Send for Coherent<'_, T> {}
// SAFETY: Sharing `&Coherent` across threads is safe if `T` is `Sync`, because all
// methods that access the buffer contents (`field_read`, `field_write`, `as_slice`,
// `as_slice_mut`) are `unsafe`, and callers are responsible for ensuring no data races occur.
// The safe methods only return metadata or raw pointers whose use requires `unsafe`.
-unsafe impl<T: KnownSize + ?Sized + AsBytes + FromBytes + Sync> Sync for Coherent<T> {}
+unsafe impl<T: KnownSize + ?Sized + AsBytes + FromBytes + Sync> Sync for Coherent<'_, T> {}
-impl<T: KnownSize + AsBytes + ?Sized> debugfs::BinaryWriter for Coherent<T> {
+impl<T: KnownSize + AsBytes + ?Sized> debugfs::BinaryWriter for Coherent<'_, T> {
fn write_to_slice(
&self,
writer: &mut UserSliceWriter,
@@ -1236,7 +1229,7 @@ fn as_view(self) -> CoherentView<'a, Self::Target> {
}
}
-impl<'a, T: ?Sized + KnownSize> IoBase<'a> for &'a Coherent<T> {
+impl<'a, T: ?Sized + KnownSize> IoBase<'a> for &'a Coherent<'_, T> {
type Backend = CoherentIoBackend;
type Target = T;
diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
index 5f6c4d7a1a51..f09078228c53 100644
--- a/rust/kernel/uaccess.rs
+++ b/rust/kernel/uaccess.rs
@@ -520,14 +520,14 @@ pub fn write_slice(&mut self, data: &[u8]) -> Result {
///
/// fn copy_dma_to_user(
/// mut writer: UserSliceWriter,
- /// alloc: &Coherent<[u8]>,
+ /// alloc: &Coherent<'_, [u8]>,
/// ) -> Result {
/// writer.write_dma(alloc, 0, 256)
/// }
/// ```
pub fn write_dma<T: KnownSize + AsBytes + ?Sized>(
&mut self,
- alloc: &Coherent<T>,
+ alloc: &Coherent<'_, T>,
offset: usize,
count: usize,
) -> Result {
diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
index 0fac9d4ae566..ffb693544673 100644
--- a/samples/rust/rust_dma.rs
+++ b/samples/rust/rust_dma.rs
@@ -34,7 +34,7 @@
#[pin_data(PinnedDrop)]
struct DmaSampleData<'bound> {
pdev: &'bound pci::Device<Bound>,
- ca: Coherent<[MyStruct]>,
+ ca: Coherent<'bound, [MyStruct]>,
#[pin]
sgt: SGTable<Owned<VVec<u8>>>,
}
@@ -86,7 +86,7 @@ fn probe<'bound>(
// SAFETY: There are no concurrent calls to DMA allocation and mapping primitives.
unsafe { pdev.dma_set_mask_and_coherent(mask)? };
- let ca: Coherent<[MyStruct]> =
+ let ca: Coherent<'_, [MyStruct]> =
Coherent::zeroed_slice(pdev.as_ref(), TEST_VALUES.len(), GFP_KERNEL)?;
for (i, value) in TEST_VALUES.into_iter().enumerate() {
--
2.55.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 4/4] rust: dma: tie Coherent and CoherentBox to the device's bound lifetime
2026-08-30 19:37 ` [PATCH 4/4] rust: dma: tie Coherent and CoherentBox to the device's bound lifetime Danilo Krummrich
@ 2026-08-30 19:47 ` sashiko-bot
2026-09-03 13:20 ` Gary Guo
1 sibling, 0 replies; 16+ messages in thread
From: sashiko-bot @ 2026-08-30 19:47 UTC (permalink / raw)
To: Danilo Krummrich; +Cc: ojeda, Danilo Krummrich, dri-devel, Alexandre Courbot
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Safe constructors of `Coherent` and `CoherentBox` allow leaking DMA allocations (e.g., via `std::mem::forget`), causing device resources like IOMMU mappings to outlive the device's bound lifetime, which can lead to Undefined Behavior upon device unbind.
--
commit 6e4d094e50aec8e58639ef61e8619143bb52d532
Author: Danilo Krummrich <dakr@kernel.org>
Subject: rust: dma: tie Coherent and CoherentBox to the device's bound lifetime
This commit adds a lifetime parameter to Coherent and CoherentBox that ties
the DMA allocation to the device's bound scope. This helps ensure it is
freed before the device is unbound.
> diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
> index 79f453e9ec0b6..4ce914b7d1dad 100644
> --- a/rust/kernel/dma.rs
> +++ b/rust/kernel/dma.rs
[ ... ]
> @@ -663,10 +656,10 @@ pub unsafe fn as_mut(&self) -> &mut T {
> }
> }
>
> -impl<T: AsBytes + FromBytes> Coherent<T> {
> +impl<'a, T: AsBytes + FromBytes> Coherent<'a, T> {
> /// Allocates a region of `T` of coherent memory.
> fn alloc_with_attrs(
> - dev: &device::Device<Bound>,
> + dev: &'a device::Device<Bound>,
> gfp_flags: kernel::alloc::Flags,
> dma_attrs: Attrs,
> ) -> Result<Self> {
[Severity: Medium]
Is it safe to expose constructors like alloc_with_attrs() as safe APIs
given that the returned object can be deliberately leaked via safe code?
The documentation on Coherent notes this is an accepted soundness hole,
but allowing a DMA allocation to be leaked (e.g., via std::mem::forget)
means device resources like IOMMU mappings can outlive the device's bound
lifetime.
If the device unbinds and its structures are torn down, could these leaked
active DMA mappings lead to memory corruption or undefined behavior?
Since exposing a safe API that can trigger undefined behavior through entirely
safe code violates standard safety guarantees, would it make sense to mark
alloc_with_attrs() and similar allocation functions as unsafe?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260830193824.471089-1-dakr@kernel.org?part=4
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] rust: debugfs: drop 'static bound from ScopedDir file creation methods
2026-08-30 19:37 ` [PATCH 1/4] rust: debugfs: drop 'static bound from ScopedDir file creation methods Danilo Krummrich
@ 2026-08-30 19:53 ` sashiko-bot
2026-09-03 13:12 ` Gary Guo
1 sibling, 0 replies; 16+ messages in thread
From: sashiko-bot @ 2026-08-30 19:53 UTC (permalink / raw)
To: Danilo Krummrich; +Cc: ojeda, Alexandre Courbot, Danilo Krummrich, dri-devel
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Unsound transmute in `FileOps::fops` leads to Stack Use-After-Free due to silent constant promotion failure for non-'static types.
- [High] Unsound reliance on RAII (Drop) to manage C API unregistration for non-'static types allows a Use-After-Free via `mem::forget`.
- [Medium] The patch missed dropping the `T: 'static` bound from `ScopedDir`'s callback-based file creation methods.
- [Low] Missing `#[inline]` annotation on the small abstraction function `fops`.
--
commit 7b2fa56e56ab7cdc906ac4f77ac3f200f9ffe65e
Author: Danilo Krummrich <dakr@kernel.org>
rust: debugfs: drop 'static bound from ScopedDir file creation methods
This commit drops the T: 'static bound from various ScopedDir file creation
methods to support registering debugfs files backed by types that contain
non-'static references. It formalizes a type invariant that FileOps instances
reside in static storage and replaces the Deref implementation with an
explicit fops() method.
> diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
> index d7b8014a64746..2beb55d444ca1 100644
> --- a/rust/kernel/debugfs.rs
> +++ b/rust/kernel/debugfs.rs
[ ... ]
> @@ -550,7 +550,7 @@ fn create_file<T: Sync>(&self, name: &CStr, data: &'data T, vtable: &'static Fil
> /// This function does not produce an owning handle to the file. The created
> /// file is removed when the [`Scope`] that this directory belongs
> /// to is dropped.
> - pub fn read_only_file<T: Writer + Send + Sync + 'static>(&self, name: &CStr, data: &'data T) {
> + pub fn read_only_file<T: Writer + Send + Sync>(&self, name: &CStr, data: &'data T) {
> self.create_file(name, data, &T::FILE_OPS)
> }
[Severity: High]
Does removing the 'static bound introduce a potential use-after-free if the
directory scope is leaked?
If a developer safely leaks a Pin<KBox<Scope<T>>> (e.g., via core::mem::forget)
where T borrows a local stack variable, the Drop-based unregistration will be
skipped. The local variables borrowed by T will still expire, but the C VFS will
retain a pointer to them, leading to a dangling pointer when the debugfs file is
later accessed.
[ ... ]
> @@ -596,11 +592,7 @@ pub fn read_callback_file<T, F>(&self, name: &CStr, data: &'data T, _f: &'static
> /// This function does not produce an owning handle to the file. The created
> /// file is removed when the [`Scope`] that this directory belongs
> /// to is dropped.
> - pub fn read_write_file<T: Writer + Reader + Send + Sync + 'static>(
> - &self,
> - name: &CStr,
> - data: &'data T,
> - ) {
> + pub fn read_write_file<T: Writer + Reader + Send + Sync>(&self, name: &CStr, data: &'data T) {
> let vtable = &<T as ReadWriteFile<_>>::FILE_OPS;
> self.create_file(name, data, vtable)
> }
[Severity: Medium]
Was it intended to leave the T: 'static bound in place for the callback-based
methods like read_callback_file() and read_write_callback_file()?
The commit states the goal is to drop the 'static bound from ScopedDir's file
creation methods, but looking at the full file, it appears it was retained on
the callback variants.
[ ... ]
> diff --git a/rust/kernel/debugfs/file_ops.rs b/rust/kernel/debugfs/file_ops.rs
> index f15908f71c4a2..7e1dd8c75ad92 100644
> --- a/rust/kernel/debugfs/file_ops.rs
> +++ b/rust/kernel/debugfs/file_ops.rs
[ ... ]
> @@ -65,11 +67,11 @@ pub(super) const fn adapt(&self) -> &FileOps<T::Inner> {
> }
>
> #[cfg(CONFIG_DEBUG_FS)]
> -impl<T> Deref for FileOps<T> {
> - type Target = bindings::file_operations;
> -
> - fn deref(&self) -> &Self::Target {
> - &self.operations
> +impl<T> FileOps<T> {
> + /// Returns a `'static` reference to the inner `file_operations`.
> + pub(crate) fn fops(&self) -> &'static bindings::file_operations {
[Severity: Low]
Does this small abstraction function need the #[inline] attribute?
The subsystem coding guidelines specify that small functions in the kernel crate
that forward to a C binding call or wrap basic data access should be annotated
with #[inline].
> + // SAFETY: By the type invariant, `self` resides in static storage.
> + unsafe { core::mem::transmute(&self.operations) }
> }
> }
[Severity: Critical]
Can this transmute cause a stack use-after-free by extending a short-lived
reference to 'static?
Since the T: 'static bound was dropped from methods like read_only_file(), using
a non-'static T will cause constant promotion to fail when evaluating
&T::FILE_OPS. This creates a temporary FileOps<T> on the local stack instead of
in static storage.
When fops() is called on this stack temporary, the transmute bypasses the actual
lifetime and returns a 'static reference to stack memory. When the stack frame
pops, the C VFS is left holding a dangling pointer.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260830193824.471089-1-dakr@kernel.org?part=1
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/4] samples: rust_dma: separate driver type from driver data
2026-08-30 19:37 ` [PATCH 3/4] samples: rust_dma: separate driver type from driver data Danilo Krummrich
@ 2026-08-30 19:57 ` sashiko-bot
2026-09-03 13:13 ` Gary Guo
1 sibling, 0 replies; 16+ messages in thread
From: sashiko-bot @ 2026-08-30 19:57 UTC (permalink / raw)
To: Danilo Krummrich; +Cc: ojeda, Alexandre Courbot, dri-devel, Danilo Krummrich
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Missing generic lifetime parameter on Generic Associated Type (GAT) `Self::Data`.
- [Low] Type mismatch between `&pci::Device<Core<'_>>` and `&pci::Device<Bound>` in struct field initialization.
--
commit e990389a909f2dd12217de05acd654c0fd95ac40
Author: Danilo Krummrich <dakr@kernel.org>
samples: rust_dma: separate driver type from driver data
This commit splits the DmaSampleDriver into a driver type and a separate
DmaSampleData struct for device private data. It updates the associated Data
type to carry a lifetime bound, preparing for future changes to
dma::Coherent.
> diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
> index bd60034ded23a..0fac9d4ae566e 100644
> --- a/samples/rust/rust_dma.rs
> +++ b/samples/rust/rust_dma.rs
[ ... ]
> @@ -67,13 +71,13 @@ unsafe impl kernel::transmute::FromBytes for MyStruct {}
>
> impl pci::Driver for DmaSampleDriver {
> type IdInfo = ();
> - type Data<'bound> = Self;
> + type Data<'bound> = DmaSampleData<'bound>;
> const ID_TABLE: pci::IdTable<Self::IdInfo> = &PCI_TABLE;
>
> fn probe<'bound>(
> pdev: &'bound pci::Device<Core<'_>>,
> _info: Option<&'bound Self::IdInfo>,
> - ) -> impl PinInit<Self, Error> + 'bound {
> + ) -> impl PinInit<Self::Data<'bound>, Error> + 'bound {
> pin_init::pin_init_scope(move || {
> dev_info!(pdev, "Probe DMA test driver.\n");
>
> @@ -94,8 +98,8 @@ fn probe<'bound>(
>
> let sgt = SGTable::new(pdev.as_ref(), pages, DataDirection::ToDevice, GFP_KERNEL);
>
> - Ok(try_pin_init!(Self {
> - pdev: pdev.into(),
> + Ok(try_pin_init!(Self::Data {
[Severity: Low]
Does this code build without explicitly providing the lifetime parameter
for the Generic Associated Type (GAT)?
Rust typically requires explicit generic parameters for GATs used in paths,
so using Self::Data here might trigger a missing generics build error.
> + pdev,
[Severity: Low]
Is there a type mismatch during the struct initialization here?
The probe() function receives pdev as &'bound pci::Device<Core<'_>>, but
the DmaSampleData struct definition expects &'bound pci::Device<Bound>:
struct DmaSampleData<'bound> {
pdev: &'bound pci::Device<Bound>,
...
}
Since implicit Deref coercions do not apply to struct field initializations,
this direct assignment could cause a compilation failure.
> ca,
> sgt <- sgt,
> }))
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260830193824.471089-1-dakr@kernel.org?part=3
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] rust: debugfs: drop 'static bound from ScopedDir file creation methods
2026-08-30 19:37 ` [PATCH 1/4] rust: debugfs: drop 'static bound from ScopedDir file creation methods Danilo Krummrich
2026-08-30 19:53 ` sashiko-bot
@ 2026-09-03 13:12 ` Gary Guo
2026-09-03 15:07 ` Danilo Krummrich
1 sibling, 1 reply; 16+ messages in thread
From: Gary Guo @ 2026-09-03 13:12 UTC (permalink / raw)
To: Danilo Krummrich, abdiel.janulgue, daniel.almeida, robin.murphy,
a.hindborg, gregkh, rafael, aliceryhl, acourbot, ojeda, boqun,
gary, bjorn3_gh, lossin, tmgross, tamird, work, mmaurer
Cc: driver-core, nova-gpu, dri-devel, linux-kernel, rust-for-linux
On Sun Aug 30, 2026 at 8:37 PM BST, Danilo Krummrich wrote:
> Drop the T: 'static bound from ScopedDir's file creation methods
> (read_binary_file(), read_only_file(), etc.) to support registering
> debugfs files backed by types that contain non-'static references, such
> as dma::Coherent<'a, T>.
>
> The previous 'static bound existed because ScopedDir::create_file() took
> &'static FileOps<T>, and &'static requires T: 'static for well-
> formedness. However, this was overly conservative; FileOps instances are
> always associated consts residing in static storage, so the pointer
> passed to the C debugfs API is always valid for the file's lifetime.
>
> Formalize this as a type invariant on FileOps. All instances reside in
> static storage, enforced by requiring FileOps::new() to only be used in
> const/static items. Replace the Deref impl with an explicit fops()
> method that returns &'static bindings::file_operations, justified by the
> type invariant.
>
> With this, ScopedDir::create_file() takes &FileOps<T> (no 'static),
> preserving the generic type safety (T links the fops to the data type)
> while allowing non-'static T.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
> rust/kernel/debugfs.rs | 26 +++++------------
> rust/kernel/debugfs/entry.rs | 4 +--
> rust/kernel/debugfs/file_ops.rs | 52 +++++++++++++++++++--------------
> 3 files changed, 39 insertions(+), 43 deletions(-)
>
> diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
> index d7b8014a6474..2beb55d444ca 100644
> --- a/rust/kernel/debugfs.rs
> +++ b/rust/kernel/debugfs.rs
> @@ -538,7 +538,7 @@ pub fn dir<'dir2>(&'dir2 self, name: &CStr) -> ScopedDir<'data, 'dir2> {
> }
> }
>
> - fn create_file<T: Sync>(&self, name: &CStr, data: &'data T, vtable: &'static FileOps<T>) {
> + fn create_file<T: Sync>(&self, name: &CStr, data: &'data T, vtable: &FileOps<T>) {
> #[cfg(CONFIG_DEBUG_FS)]
> core::mem::forget(Entry::file(name, &self.entry, data, vtable));
With the signature change you're relying on static promotion to happen -- which
would still happen without a lifetime bound, but I find it somewhat
uncomfortable relying on that fact without a lifetime check.
> }
> @@ -550,7 +550,7 @@ fn create_file<T: Sync>(&self, name: &CStr, data: &'data T, vtable: &'static Fil
> /// This function does not produce an owning handle to the file. The created
> /// file is removed when the [`Scope`] that this directory belongs
> /// to is dropped.
> - pub fn read_only_file<T: Writer + Send + Sync + 'static>(&self, name: &CStr, data: &'data T) {
> + pub fn read_only_file<T: Writer + Send + Sync>(&self, name: &CStr, data: &'data T) {
> self.create_file(name, data, &T::FILE_OPS)
> }
>
> @@ -560,11 +560,7 @@ pub fn read_only_file<T: Writer + Send + Sync + 'static>(&self, name: &CStr, dat
> ///
> /// This function does not produce an owning handle to the file. The created file is removed
> /// when the [`Scope`] that this directory belongs to is dropped.
> - pub fn read_binary_file<T: BinaryWriter + Send + Sync + 'static>(
> - &self,
> - name: &CStr,
> - data: &'data T,
> - ) {
> + pub fn read_binary_file<T: BinaryWriter + Send + Sync>(&self, name: &CStr, data: &'data T) {
> self.create_file(name, data, &T::FILE_OPS)
> }
>
> @@ -596,11 +592,7 @@ pub fn read_callback_file<T, F>(&self, name: &CStr, data: &'data T, _f: &'static
> /// This function does not produce an owning handle to the file. The created
> /// file is removed when the [`Scope`] that this directory belongs
> /// to is dropped.
> - pub fn read_write_file<T: Writer + Reader + Send + Sync + 'static>(
> - &self,
> - name: &CStr,
> - data: &'data T,
> - ) {
> + pub fn read_write_file<T: Writer + Reader + Send + Sync>(&self, name: &CStr, data: &'data T) {
> let vtable = &<T as ReadWriteFile<_>>::FILE_OPS;
> self.create_file(name, data, vtable)
> }
> @@ -612,7 +604,7 @@ pub fn read_write_file<T: Writer + Reader + Send + Sync + 'static>(
> ///
> /// This function does not produce an owning handle to the file. The created file is removed
> /// when the [`Scope`] that this directory belongs to is dropped.
> - pub fn read_write_binary_file<T: BinaryWriter + BinaryReader + Send + Sync + 'static>(
> + pub fn read_write_binary_file<T: BinaryWriter + BinaryReader + Send + Sync>(
> &self,
> name: &CStr,
> data: &'data T,
> @@ -655,7 +647,7 @@ pub fn read_write_callback_file<T, F, W>(
> /// This function does not produce an owning handle to the file. The created
> /// file is removed when the [`Scope`] that this directory belongs
> /// to is dropped.
> - pub fn write_only_file<T: Reader + Send + Sync + 'static>(&self, name: &CStr, data: &'data T) {
> + pub fn write_only_file<T: Reader + Send + Sync>(&self, name: &CStr, data: &'data T) {
> let vtable = &<T as WriteFile<_>>::FILE_OPS;
> self.create_file(name, data, vtable)
> }
> @@ -666,11 +658,7 @@ pub fn write_only_file<T: Reader + Send + Sync + 'static>(&self, name: &CStr, da
> ///
> /// This function does not produce an owning handle to the file. The created file is removed
> /// when the [`Scope`] that this directory belongs to is dropped.
> - pub fn write_binary_file<T: BinaryReader + Send + Sync + 'static>(
> - &self,
> - name: &CStr,
> - data: &'data T,
> - ) {
> + pub fn write_binary_file<T: BinaryReader + Send + Sync>(&self, name: &CStr, data: &'data T) {
> self.create_file(name, data, &T::FILE_OPS)
> }
>
> diff --git a/rust/kernel/debugfs/entry.rs b/rust/kernel/debugfs/entry.rs
> index 46aad64896ec..88a870d8c295 100644
> --- a/rust/kernel/debugfs/entry.rs
> +++ b/rust/kernel/debugfs/entry.rs
> @@ -74,7 +74,7 @@ pub(crate) unsafe fn dynamic_file<T>(
> parent.as_ptr(),
> core::ptr::from_ref(data) as *mut c_void,
> core::ptr::null(),
> - &**file_ops,
> + file_ops.fops(),
> )
> };
>
> @@ -127,7 +127,7 @@ pub(crate) fn file<T>(
> parent.as_ptr(),
> core::ptr::from_ref(data) as *mut c_void,
> core::ptr::null(),
> - &**file_ops,
> + file_ops.fops(),
> )
> };
>
> diff --git a/rust/kernel/debugfs/file_ops.rs b/rust/kernel/debugfs/file_ops.rs
> index f15908f71c4a..7e1dd8c75ad9 100644
> --- a/rust/kernel/debugfs/file_ops.rs
> +++ b/rust/kernel/debugfs/file_ops.rs
> @@ -20,14 +20,12 @@
>
> use core::marker::PhantomData;
>
> -#[cfg(CONFIG_DEBUG_FS)]
> -use core::ops::Deref;
> -
> -/// # Invariant
> +/// # Invariants
> ///
> -/// `FileOps<T>` will always contain an `operations` which is safe to use for a file backed
> -/// off an inode which has a pointer to a `T` in its private data that is safe to convert
> -/// into a reference.
> +/// - `FileOps<T>` will always contain an `operations` which is safe to use for a file backed
> +/// off an inode which has a pointer to a `T` in its private data that is safe to convert
> +/// into a reference.
> +/// - Every instance of `FileOps<T>` resides in static storage.
This can be better done by storing `&'static bindings::file_operations` in
`FileOps<T>` instead of just by value. That is actually better than the current
impl, IMO, because `mode` for example doesn't have to be in static storage. (You
can also then make `FileOps<T>` `Copy`).
If you made the change, you'd still have `&'static` checked at compile time, and
the fops below can be safe.
Best,
Gary
> pub(super) struct FileOps<T> {
> #[cfg(CONFIG_DEBUG_FS)]
> operations: bindings::file_operations,
> @@ -39,9 +37,13 @@ pub(super) struct FileOps<T> {
> impl<T> FileOps<T> {
> /// # Safety
> ///
> - /// The caller asserts that the provided `operations` is safe to use for a file whose
> - /// inode has a pointer to `T` in its private data that is safe to convert into a reference.
> + /// - The caller asserts that the provided `operations` is safe to use for a file whose
> + /// inode has a pointer to `T` in its private data that is safe to convert into a reference.
> + /// - Must only be used to initialize a `const` or `static` item, to uphold the type invariant
> + /// that all `FileOps` instances reside in static storage.
> const unsafe fn new(operations: bindings::file_operations, mode: u16) -> Self {
> + // INVARIANT: The caller is required to only use this in a `const` or `static` item,
> + // ensuring that all `FileOps` instances reside in static storage.
> Self {
> #[cfg(CONFIG_DEBUG_FS)]
> operations,
> @@ -65,11 +67,11 @@ pub(super) const fn adapt(&self) -> &FileOps<T::Inner> {
> }
>
> #[cfg(CONFIG_DEBUG_FS)]
> -impl<T> Deref for FileOps<T> {
> - type Target = bindings::file_operations;
> -
> - fn deref(&self) -> &Self::Target {
> - &self.operations
> +impl<T> FileOps<T> {
> + /// Returns a `'static` reference to the inner `file_operations`.
> + pub(crate) fn fops(&self) -> &'static bindings::file_operations {
> + // SAFETY: By the type invariant, `self` resides in static storage.
> + unsafe { core::mem::transmute(&self.operations) }
> }
> }
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/4] rust: dma: tie CoherentHandle to the device's bound lifetime
2026-08-30 19:37 ` [PATCH 2/4] rust: dma: tie CoherentHandle to the device's bound lifetime Danilo Krummrich
@ 2026-09-03 13:12 ` Gary Guo
0 siblings, 0 replies; 16+ messages in thread
From: Gary Guo @ 2026-09-03 13:12 UTC (permalink / raw)
To: Danilo Krummrich, abdiel.janulgue, daniel.almeida, robin.murphy,
a.hindborg, gregkh, rafael, aliceryhl, acourbot, ojeda, boqun,
gary, bjorn3_gh, lossin, tmgross, tamird, work, mmaurer
Cc: driver-core, nova-gpu, dri-devel, linux-kernel, rust-for-linux
On Sun Aug 30, 2026 at 8:37 PM BST, Danilo Krummrich wrote:
> Add a lifetime parameter to CoherentHandle that ties the DMA allocation
> to the device's bound scope, ensuring it is freed before the device is
> unbound.
>
> DMA allocations carry device resources (e.g. IOMMU mappings) that must
> not outlive the device's bound lifetime. Without a lifetime parameter,
> there was no compile-time enforcement that a CoherentHandle is dropped
> before the device is unbound.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
Reviewed-by: Gary Guo <gary@garyguo.net>
> ---
> drivers/gpu/nova-core/fb.rs | 2 +-
> rust/kernel/dma.rs | 20 ++++++++++----------
> 2 files changed, 11 insertions(+), 11 deletions(-)
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/4] samples: rust_dma: separate driver type from driver data
2026-08-30 19:37 ` [PATCH 3/4] samples: rust_dma: separate driver type from driver data Danilo Krummrich
2026-08-30 19:57 ` sashiko-bot
@ 2026-09-03 13:13 ` Gary Guo
1 sibling, 0 replies; 16+ messages in thread
From: Gary Guo @ 2026-09-03 13:13 UTC (permalink / raw)
To: Danilo Krummrich, abdiel.janulgue, daniel.almeida, robin.murphy,
a.hindborg, gregkh, rafael, aliceryhl, acourbot, ojeda, boqun,
gary, bjorn3_gh, lossin, tmgross, tamird, work, mmaurer
Cc: driver-core, nova-gpu, dri-devel, linux-kernel, rust-for-linux
On Sun Aug 30, 2026 at 8:37 PM BST, Danilo Krummrich wrote:
> Split DmaSampleDriver into a driver type and a separate DmaSampleData
> struct for the driver's bus device private data, using
> DmaSampleData<'bound> as the Driver::Data<'bound> associated type.
>
> Store a &'bound pci::Device<Bound> reference instead of an
> ARef<pci::Device>, tying the data to the device's bound scope.
>
> This prepares for adding a lifetime parameter to dma::Coherent, which
> requires the data type to carry a lifetime.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
Reviewed-by: Gary Guo <gary@garyguo.net>
> ---
> samples/rust/rust_dma.rs | 26 +++++++++++++++-----------
> 1 file changed, 15 insertions(+), 11 deletions(-)
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 4/4] rust: dma: tie Coherent and CoherentBox to the device's bound lifetime
2026-08-30 19:37 ` [PATCH 4/4] rust: dma: tie Coherent and CoherentBox to the device's bound lifetime Danilo Krummrich
2026-08-30 19:47 ` sashiko-bot
@ 2026-09-03 13:20 ` Gary Guo
2026-09-03 15:22 ` Danilo Krummrich
1 sibling, 1 reply; 16+ messages in thread
From: Gary Guo @ 2026-09-03 13:20 UTC (permalink / raw)
To: Danilo Krummrich, abdiel.janulgue, daniel.almeida, robin.murphy,
a.hindborg, gregkh, rafael, aliceryhl, acourbot, ojeda, boqun,
gary, bjorn3_gh, lossin, tmgross, tamird, work, mmaurer
Cc: driver-core, nova-gpu, dri-devel, linux-kernel, rust-for-linux
On Sun Aug 30, 2026 at 8:37 PM BST, Danilo Krummrich wrote:
> Add a lifetime parameter to Coherent and CoherentBox that ties the DMA
> allocation to the device's bound scope, ensuring it is freed before the
> device is unbound.
>
> DMA allocations carry device resources (e.g. IOMMU mappings) that must
> not outlive the device's bound lifetime. Without a lifetime parameter,
> there was no compile-time enforcement that a Coherent or CoherentBox is
> dropped before the device is unbound.
>
> Propagate the new lifetime parameter through all users.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
The rust/kernel code looks good to me. Haven't checked nova part in detail, but
it looks like a mechanical conversion, so would be fine if it builds.
Reviewed-by: Gary Guo <gary@garyguo.net>
Sashiko points out that the `Coherent` could be leaked -- what's the implication
when that happens? I think it's not going to be as problematic like
registrations because coherent allocation carries no callbacks, so we probably
don't need this to be unsafe, but I do wonder how'd DMA subsystem handle this.
Best,
Gary
> ---
> drivers/gpu/nova-core/falcon.rs | 2 +-
> drivers/gpu/nova-core/fb.rs | 2 +-
> drivers/gpu/nova-core/firmware/booter.rs | 2 +-
> drivers/gpu/nova-core/firmware/fsp.rs | 8 +-
> .../nova-core/firmware/fwsec/bootloader.rs | 12 +-
> drivers/gpu/nova-core/firmware/gsp.rs | 14 +-
> drivers/gpu/nova-core/firmware/riscv.rs | 8 +-
> drivers/gpu/nova-core/fsp.rs | 20 +--
> drivers/gpu/nova-core/gpu.rs | 4 +-
> drivers/gpu/nova-core/gsp.rs | 30 ++---
> drivers/gpu/nova-core/gsp/boot.rs | 10 +-
> drivers/gpu/nova-core/gsp/cmdq.rs | 35 +++--
> drivers/gpu/nova-core/gsp/commands.rs | 2 +-
> drivers/gpu/nova-core/gsp/fw.rs | 12 +-
> drivers/gpu/nova-core/gsp/hal.rs | 14 +-
> drivers/gpu/nova-core/gsp/hal/gh100.rs | 16 +--
> drivers/gpu/nova-core/gsp/hal/tu102.rs | 34 ++---
> drivers/gpu/nova-core/gsp/sequencer.rs | 6 +-
> rust/kernel/dma.rs | 121 +++++++++---------
> rust/kernel/uaccess.rs | 4 +-
> samples/rust/rust_dma.rs | 4 +-
> 21 files changed, 176 insertions(+), 184 deletions(-)
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] rust: debugfs: drop 'static bound from ScopedDir file creation methods
2026-09-03 13:12 ` Gary Guo
@ 2026-09-03 15:07 ` Danilo Krummrich
2026-09-03 15:16 ` Gary Guo
0 siblings, 1 reply; 16+ messages in thread
From: Danilo Krummrich @ 2026-09-03 15:07 UTC (permalink / raw)
To: Gary Guo
Cc: abdiel.janulgue, daniel.almeida, robin.murphy, a.hindborg, gregkh,
rafael, aliceryhl, acourbot, ojeda, boqun, bjorn3_gh, lossin,
tmgross, tamird, work, mmaurer, driver-core, nova-gpu, dri-devel,
linux-kernel, rust-for-linux
On Thu Sep 3, 2026 at 3:12 PM CEST, Gary Guo wrote:
> This can be better done by storing `&'static bindings::file_operations` in
> `FileOps<T>` instead of just by value. That is actually better than the current
> impl, IMO, because `mode` for example doesn't have to be in static storage. (You
> can also then make `FileOps<T>` `Copy`).
That's a great suggestion, thanks. It simplifies the patch to:
Author: Danilo Krummrich <dakr@kernel.org>
Date: Sat Aug 29 14:42:54 2026 +0200
rust: debugfs: drop 'static bound from ScopedDir file creation methods
Drop the T: 'static bound from ScopedDir's file creation methods
(read_binary_file(), read_only_file(), etc.) to support registering
debugfs files backed by types that contain non-'static references, such
as dma::Coherent<'a, T>.
The previous 'static bound existed because ScopedDir::create_file() took
&'static FileOps<T>, and &'static requires T: 'static for well-
formedness. However, this was overly conservative: the file_operations
pointer passed to the C debugfs API just needs to be 'static, not the
entire FileOps<T>.
Store &'static bindings::file_operations in FileOps<T> instead of the
file_operations by value. In each trait impl, take a reference to the
file_operations struct within the const block; since
bindings::file_operations does not mention T, the reference is promoted
to 'static regardless of T's lifetime parameters.
Replace the Deref impl with an explicit fops() method that returns the
stored &'static reference.
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
index d7b8014a6474..2beb55d444ca 100644
--- a/rust/kernel/debugfs.rs
+++ b/rust/kernel/debugfs.rs
@@ -538,7 +538,7 @@ pub fn dir<'dir2>(&'dir2 self, name: &CStr) -> ScopedDir<'data, 'dir2> {
}
}
- fn create_file<T: Sync>(&self, name: &CStr, data: &'data T, vtable: &'static FileOps<T>) {
+ fn create_file<T: Sync>(&self, name: &CStr, data: &'data T, vtable: &FileOps<T>) {
#[cfg(CONFIG_DEBUG_FS)]
core::mem::forget(Entry::file(name, &self.entry, data, vtable));
}
@@ -550,7 +550,7 @@ fn create_file<T: Sync>(&self, name: &CStr, data: &'data T, vtable: &'static Fil
/// This function does not produce an owning handle to the file. The created
/// file is removed when the [`Scope`] that this directory belongs
/// to is dropped.
- pub fn read_only_file<T: Writer + Send + Sync + 'static>(&self, name: &CStr, data: &'data T) {
+ pub fn read_only_file<T: Writer + Send + Sync>(&self, name: &CStr, data: &'data T) {
self.create_file(name, data, &T::FILE_OPS)
}
@@ -560,11 +560,7 @@ pub fn read_only_file<T: Writer + Send + Sync + 'static>(&self, name: &CStr, dat
///
/// This function does not produce an owning handle to the file. The created file is removed
/// when the [`Scope`] that this directory belongs to is dropped.
- pub fn read_binary_file<T: BinaryWriter + Send + Sync + 'static>(
- &self,
- name: &CStr,
- data: &'data T,
- ) {
+ pub fn read_binary_file<T: BinaryWriter + Send + Sync>(&self, name: &CStr, data: &'data T) {
self.create_file(name, data, &T::FILE_OPS)
}
@@ -596,11 +592,7 @@ pub fn read_callback_file<T, F>(&self, name: &CStr, data: &'data T, _f: &'static
/// This function does not produce an owning handle to the file. The created
/// file is removed when the [`Scope`] that this directory belongs
/// to is dropped.
- pub fn read_write_file<T: Writer + Reader + Send + Sync + 'static>(
- &self,
- name: &CStr,
- data: &'data T,
- ) {
+ pub fn read_write_file<T: Writer + Reader + Send + Sync>(&self, name: &CStr, data: &'data T) {
let vtable = &<T as ReadWriteFile<_>>::FILE_OPS;
self.create_file(name, data, vtable)
}
@@ -612,7 +604,7 @@ pub fn read_write_file<T: Writer + Reader + Send + Sync + 'static>(
///
/// This function does not produce an owning handle to the file. The created file is removed
/// when the [`Scope`] that this directory belongs to is dropped.
- pub fn read_write_binary_file<T: BinaryWriter + BinaryReader + Send + Sync + 'static>(
+ pub fn read_write_binary_file<T: BinaryWriter + BinaryReader + Send + Sync>(
&self,
name: &CStr,
data: &'data T,
@@ -655,7 +647,7 @@ pub fn read_write_callback_file<T, F, W>(
/// This function does not produce an owning handle to the file. The created
/// file is removed when the [`Scope`] that this directory belongs
/// to is dropped.
- pub fn write_only_file<T: Reader + Send + Sync + 'static>(&self, name: &CStr, data: &'data T) {
+ pub fn write_only_file<T: Reader + Send + Sync>(&self, name: &CStr, data: &'data T) {
let vtable = &<T as WriteFile<_>>::FILE_OPS;
self.create_file(name, data, vtable)
}
@@ -666,11 +658,7 @@ pub fn write_only_file<T: Reader + Send + Sync + 'static>(&self, name: &CStr, da
///
/// This function does not produce an owning handle to the file. The created file is removed
/// when the [`Scope`] that this directory belongs to is dropped.
- pub fn write_binary_file<T: BinaryReader + Send + Sync + 'static>(
- &self,
- name: &CStr,
- data: &'data T,
- ) {
+ pub fn write_binary_file<T: BinaryReader + Send + Sync>(&self, name: &CStr, data: &'data T) {
self.create_file(name, data, &T::FILE_OPS)
}
diff --git a/rust/kernel/debugfs/entry.rs b/rust/kernel/debugfs/entry.rs
index 46aad64896ec..88a870d8c295 100644
--- a/rust/kernel/debugfs/entry.rs
+++ b/rust/kernel/debugfs/entry.rs
@@ -74,7 +74,7 @@ pub(crate) unsafe fn dynamic_file<T>(
parent.as_ptr(),
core::ptr::from_ref(data) as *mut c_void,
core::ptr::null(),
- &**file_ops,
+ file_ops.fops(),
)
};
@@ -127,7 +127,7 @@ pub(crate) fn file<T>(
parent.as_ptr(),
core::ptr::from_ref(data) as *mut c_void,
core::ptr::null(),
- &**file_ops,
+ file_ops.fops(),
)
};
diff --git a/rust/kernel/debugfs/file_ops.rs b/rust/kernel/debugfs/file_ops.rs
index f15908f71c4a..5c16a3196ca2 100644
--- a/rust/kernel/debugfs/file_ops.rs
+++ b/rust/kernel/debugfs/file_ops.rs
@@ -20,9 +20,6 @@
use core::marker::PhantomData;
-#[cfg(CONFIG_DEBUG_FS)]
-use core::ops::Deref;
-
/// # Invariant
///
/// `FileOps<T>` will always contain an `operations` which is safe to use for a file backed
@@ -30,7 +27,7 @@
/// into a reference.
pub(super) struct FileOps<T> {
#[cfg(CONFIG_DEBUG_FS)]
- operations: bindings::file_operations,
+ operations: &'static bindings::file_operations,
#[cfg(CONFIG_DEBUG_FS)]
mode: u16,
_phantom: PhantomData<T>,
@@ -41,7 +38,7 @@ impl<T> FileOps<T> {
///
/// The caller asserts that the provided `operations` is safe to use for a file whose
/// inode has a pointer to `T` in its private data that is safe to convert into a reference.
- const unsafe fn new(operations: bindings::file_operations, mode: u16) -> Self {
+ const unsafe fn new(operations: &'static bindings::file_operations, mode: u16) -> Self {
Self {
#[cfg(CONFIG_DEBUG_FS)]
operations,
@@ -65,11 +62,11 @@ pub(super) const fn adapt(&self) -> &FileOps<T::Inner> {
}
#[cfg(CONFIG_DEBUG_FS)]
-impl<T> Deref for FileOps<T> {
- type Target = bindings::file_operations;
-
- fn deref(&self) -> &Self::Target {
- &self.operations
+impl<T> FileOps<T> {
+ /// Returns a `'static` reference to the inner `file_operations`.
+ #[inline]
+ pub(crate) fn fops(&self) -> &'static bindings::file_operations {
+ self.operations
}
}
@@ -130,11 +127,11 @@ pub(crate) trait ReadFile<T> {
impl<T: Writer + Sync> ReadFile<T> for T {
const FILE_OPS: FileOps<T> = {
- let operations = bindings::file_operations {
+ let operations = &bindings::file_operations {
read: Some(bindings::seq_read),
llseek: Some(bindings::seq_lseek),
release: Some(bindings::single_release),
- open: Some(writer_open::<Self>),
+ open: Some(writer_open::<T>),
..pin_init::zeroed()
};
// SAFETY: `operations` is all stock `seq_file` implementations except for `writer_open`.
@@ -181,7 +178,7 @@ pub(crate) trait ReadWriteFile<T> {
impl<T: Writer + Reader + Sync> ReadWriteFile<T> for T {
const FILE_OPS: FileOps<T> = {
- let operations = bindings::file_operations {
+ let operations = &bindings::file_operations {
open: Some(writer_open::<T>),
read: Some(bindings::seq_read),
write: Some(write::<T>),
@@ -238,7 +235,7 @@ pub(crate) trait WriteFile<T> {
impl<T: Reader + Sync> WriteFile<T> for T {
const FILE_OPS: FileOps<T> = {
- let operations = bindings::file_operations {
+ let operations = &bindings::file_operations {
open: Some(write_only_open),
write: Some(write_only_write::<T>),
llseek: Some(bindings::noop_llseek),
@@ -290,7 +287,7 @@ pub(crate) trait BinaryReadFile<T> {
impl<T: BinaryWriter + Sync> BinaryReadFile<T> for T {
const FILE_OPS: FileOps<T> = {
- let operations = bindings::file_operations {
+ let operations = &bindings::file_operations {
read: Some(blob_read::<T>),
llseek: Some(bindings::default_llseek),
open: Some(bindings::simple_open),
@@ -344,7 +341,7 @@ pub(crate) trait BinaryWriteFile<T> {
impl<T: BinaryReader + Sync> BinaryWriteFile<T> for T {
const FILE_OPS: FileOps<T> = {
- let operations = bindings::file_operations {
+ let operations = &bindings::file_operations {
write: Some(blob_write::<T>),
llseek: Some(bindings::default_llseek),
open: Some(bindings::simple_open),
@@ -368,7 +365,7 @@ pub(crate) trait BinaryReadWriteFile<T> {
impl<T: BinaryWriter + BinaryReader + Sync> BinaryReadWriteFile<T> for T {
const FILE_OPS: FileOps<T> = {
- let operations = bindings::file_operations {
+ let operations = &bindings::file_operations {
read: Some(blob_read::<T>),
write: Some(blob_write::<T>),
llseek: Some(bindings::default_llseek),
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] rust: debugfs: drop 'static bound from ScopedDir file creation methods
2026-09-03 15:07 ` Danilo Krummrich
@ 2026-09-03 15:16 ` Gary Guo
0 siblings, 0 replies; 16+ messages in thread
From: Gary Guo @ 2026-09-03 15:16 UTC (permalink / raw)
To: Danilo Krummrich, Gary Guo
Cc: abdiel.janulgue, daniel.almeida, robin.murphy, a.hindborg, gregkh,
rafael, aliceryhl, acourbot, ojeda, boqun, bjorn3_gh, lossin,
tmgross, tamird, work, mmaurer, driver-core, nova-gpu, dri-devel,
linux-kernel, rust-for-linux
On Thu Sep 3, 2026 at 4:07 PM BST, Danilo Krummrich wrote:
> On Thu Sep 3, 2026 at 3:12 PM CEST, Gary Guo wrote:
>> This can be better done by storing `&'static bindings::file_operations` in
>> `FileOps<T>` instead of just by value. That is actually better than the current
>> impl, IMO, because `mode` for example doesn't have to be in static storage. (You
>> can also then make `FileOps<T>` `Copy`).
>
> That's a great suggestion, thanks. It simplifies the patch to:
>
> Author: Danilo Krummrich <dakr@kernel.org>
> Date: Sat Aug 29 14:42:54 2026 +0200
>
> rust: debugfs: drop 'static bound from ScopedDir file creation methods
>
> Drop the T: 'static bound from ScopedDir's file creation methods
> (read_binary_file(), read_only_file(), etc.) to support registering
> debugfs files backed by types that contain non-'static references, such
> as dma::Coherent<'a, T>.
>
> The previous 'static bound existed because ScopedDir::create_file() took
> &'static FileOps<T>, and &'static requires T: 'static for well-
> formedness. However, this was overly conservative: the file_operations
> pointer passed to the C debugfs API just needs to be 'static, not the
> entire FileOps<T>.
>
> Store &'static bindings::file_operations in FileOps<T> instead of the
> file_operations by value. In each trait impl, take a reference to the
> file_operations struct within the const block; since
> bindings::file_operations does not mention T, the reference is promoted
> to 'static regardless of T's lifetime parameters.
>
> Replace the Deref impl with an explicit fops() method that returns the
> stored &'static reference.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
Reviewed-by: Gary Guo <gary@garyguo.net>
Some nits below.
>
> diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
> index d7b8014a6474..2beb55d444ca 100644
> --- a/rust/kernel/debugfs.rs
> +++ b/rust/kernel/debugfs.rs
> @@ -538,7 +538,7 @@ pub fn dir<'dir2>(&'dir2 self, name: &CStr) -> ScopedDir<'data, 'dir2> {
> }
> }
>
> - fn create_file<T: Sync>(&self, name: &CStr, data: &'data T, vtable: &'static FileOps<T>) {
> + fn create_file<T: Sync>(&self, name: &CStr, data: &'data T, vtable: &FileOps<T>) {
I suppose this doesn't need to use reference anymore, but you want to keep the
diff small.
> #[cfg(CONFIG_DEBUG_FS)]
> core::mem::forget(Entry::file(name, &self.entry, data, vtable));
> }
> @@ -550,7 +550,7 @@ fn create_file<T: Sync>(&self, name: &CStr, data: &'data T, vtable: &'static Fil
> /// This function does not produce an owning handle to the file. The created
> /// file is removed when the [`Scope`] that this directory belongs
> /// to is dropped.
> - pub fn read_only_file<T: Writer + Send + Sync + 'static>(&self, name: &CStr, data: &'data T) {
> + pub fn read_only_file<T: Writer + Send + Sync>(&self, name: &CStr, data: &'data T) {
> self.create_file(name, data, &T::FILE_OPS)
> }
>
> [snip]
>
> diff --git a/rust/kernel/debugfs/file_ops.rs b/rust/kernel/debugfs/file_ops.rs
> index f15908f71c4a..5c16a3196ca2 100644
> --- a/rust/kernel/debugfs/file_ops.rs
> +++ b/rust/kernel/debugfs/file_ops.rs
> @@ -20,9 +20,6 @@
>
> use core::marker::PhantomData;
>
> -#[cfg(CONFIG_DEBUG_FS)]
> -use core::ops::Deref;
> -
> /// # Invariant
> ///
> /// `FileOps<T>` will always contain an `operations` which is safe to use for a file backed
> @@ -30,7 +27,7 @@
> /// into a reference.
> pub(super) struct FileOps<T> {
> #[cfg(CONFIG_DEBUG_FS)]
> - operations: bindings::file_operations,
> + operations: &'static bindings::file_operations,
> #[cfg(CONFIG_DEBUG_FS)]
> mode: u16,
> _phantom: PhantomData<T>,
> @@ -41,7 +38,7 @@ impl<T> FileOps<T> {
> ///
> /// The caller asserts that the provided `operations` is safe to use for a file whose
> /// inode has a pointer to `T` in its private data that is safe to convert into a reference.
> - const unsafe fn new(operations: bindings::file_operations, mode: u16) -> Self {
> + const unsafe fn new(operations: &'static bindings::file_operations, mode: u16) -> Self {
> Self {
> #[cfg(CONFIG_DEBUG_FS)]
> operations,
> @@ -65,11 +62,11 @@ pub(super) const fn adapt(&self) -> &FileOps<T::Inner> {
> }
>
> #[cfg(CONFIG_DEBUG_FS)]
> -impl<T> Deref for FileOps<T> {
> - type Target = bindings::file_operations;
> -
> - fn deref(&self) -> &Self::Target {
> - &self.operations
> +impl<T> FileOps<T> {
> + /// Returns a `'static` reference to the inner `file_operations`.
> + #[inline]
> + pub(crate) fn fops(&self) -> &'static bindings::file_operations {
> + self.operations
> }
> }
>
> @@ -130,11 +127,11 @@ pub(crate) trait ReadFile<T> {
>
> impl<T: Writer + Sync> ReadFile<T> for T {
> const FILE_OPS: FileOps<T> = {
> - let operations = bindings::file_operations {
> + let operations = &bindings::file_operations {
> read: Some(bindings::seq_read),
> llseek: Some(bindings::seq_lseek),
> release: Some(bindings::single_release),
> - open: Some(writer_open::<Self>),
> + open: Some(writer_open::<T>),
Is this change needed?
Best,
Gary
> ..pin_init::zeroed()
> };
> // SAFETY: `operations` is all stock `seq_file` implementations except for `writer_open`.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 4/4] rust: dma: tie Coherent and CoherentBox to the device's bound lifetime
2026-09-03 13:20 ` Gary Guo
@ 2026-09-03 15:22 ` Danilo Krummrich
2026-09-03 15:42 ` Gary Guo
0 siblings, 1 reply; 16+ messages in thread
From: Danilo Krummrich @ 2026-09-03 15:22 UTC (permalink / raw)
To: Gary Guo
Cc: abdiel.janulgue, daniel.almeida, robin.murphy, a.hindborg, gregkh,
rafael, aliceryhl, acourbot, ojeda, boqun, bjorn3_gh, lossin,
tmgross, tamird, work, mmaurer, driver-core, nova-gpu, dri-devel,
linux-kernel, rust-for-linux
On Thu Sep 3, 2026 at 3:20 PM CEST, Gary Guo wrote:
> The rust/kernel code looks good to me. Haven't checked nova part in detail, but
> it looks like a mechanical conversion, so would be fine if it builds.
>
> Reviewed-by: Gary Guo <gary@garyguo.net>
>
> Sashiko points out that the `Coherent` could be leaked -- what's the implication
> when that happens? I think it's not going to be as problematic like
> registrations because coherent allocation carries no callbacks, so we probably
> don't need this to be unsafe, but I do wonder how'd DMA subsystem handle this.
The implication if leaked is the same as if it is kept alive past driver unbind,
which is why I changed the TODO comment accordingly in the hunk below.
If you look for a specific example, there's [1] for instance. So, it is
problematic, which is why I added the TODO comment back then.
But, we did accept this soundness hole from the get-go for both, keeping a
coherent allocation alive beyond driver unbind and for leaking it.
With this patch it is now impossible to keep it alive beyond driver unbind, so
switching to unsafe now would be a bit odd. :)
(The fact that we did accept this for coherent allocations is also one reason
why I was recently arguing that we can also make the forget() issue an accepted
soundness hole for registrations.)
[1] https://lore.kernel.org/all/6a7910da.9c11d2ce.289b96.00da.GAE@google.com/
@@ -588,26 +587,20 @@ fn from(value: CoherentBox<T>) -> Self {
/// to an allocated region of coherent memory and `dma_addr` is the DMA address base of the
/// region.
/// - The size in bytes of the allocation is equal to size information via pointer.
-// TODO
//
-// DMA allocations potentially carry device resources (e.g.IOMMU mappings), hence for soundness
-// reasons DMA allocation would need to be embedded in a `Devres` container, in order to ensure
-// that device resources can never survive device unbind.
-//
-// However, it is neither desirable nor necessary to protect the allocated memory of the DMA
-// allocation from surviving device unbind; it would require RCU read side critical sections to
-// access the memory, which may require subsequent unnecessary copies.
-//
-// Hence, find a way to revoke the device resources of a `Coherent`, but not the
-// entire `Coherent` including the allocated memory itself.
-pub struct Coherent<T: KnownSize + ?Sized> {
- dev: ARef<device::Device>,
+// The lifetime parameter ties DMA allocations to the device's bound scope, ensuring they are freed
+// before the device is unbound under normal circumstances. However, if a `Coherent` is leaked (e.g.
+// via `mem::forget`), device resources such as IOMMU mappings will not be released. Making all
+// constructors `unsafe` to prevent this is considered too restrictive for the common case; this
+// soundness hole is accepted for now.
+pub struct Coherent<'a, T: KnownSize + ?Sized> {
+ dev: &'a device::Device<Bound>,
dma_addr: DmaAddress,
cpu_addr: NonNull<T>,
dma_attrs: Attrs,
}
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 4/4] rust: dma: tie Coherent and CoherentBox to the device's bound lifetime
2026-09-03 15:22 ` Danilo Krummrich
@ 2026-09-03 15:42 ` Gary Guo
0 siblings, 0 replies; 16+ messages in thread
From: Gary Guo @ 2026-09-03 15:42 UTC (permalink / raw)
To: Danilo Krummrich, Gary Guo
Cc: abdiel.janulgue, daniel.almeida, robin.murphy, a.hindborg, gregkh,
rafael, aliceryhl, acourbot, ojeda, boqun, bjorn3_gh, lossin,
tmgross, tamird, work, mmaurer, driver-core, nova-gpu, dri-devel,
linux-kernel, rust-for-linux
On Thu Sep 3, 2026 at 4:22 PM BST, Danilo Krummrich wrote:
> On Thu Sep 3, 2026 at 3:20 PM CEST, Gary Guo wrote:
>> The rust/kernel code looks good to me. Haven't checked nova part in detail, but
>> it looks like a mechanical conversion, so would be fine if it builds.
>>
>> Reviewed-by: Gary Guo <gary@garyguo.net>
>>
>> Sashiko points out that the `Coherent` could be leaked -- what's the implication
>> when that happens? I think it's not going to be as problematic like
>> registrations because coherent allocation carries no callbacks, so we probably
>> don't need this to be unsafe, but I do wonder how'd DMA subsystem handle this.
>
> The implication if leaked is the same as if it is kept alive past driver unbind,
> which is why I changed the TODO comment accordingly in the hunk below.
>
> If you look for a specific example, there's [1] for instance. So, it is
> problematic, which is why I added the TODO comment back then.
Right, so despite that `Coherent` itself not having callbacks, IOMMU side can
have callback that associated with `Coherent` instance and thus require device
to be bound.
>
> But, we did accept this soundness hole from the get-go for both, keeping a
> coherent allocation alive beyond driver unbind and for leaking it.
>
> With this patch it is now impossible to keep it alive beyond driver unbind, so
> switching to unsafe now would be a bit odd. :)
No, I don't want this to be unsafe.
I just wonder if it could be made not unsound at all. I think in this case at
least it is possible with a revocation mechanism -- the `Coherent` itself is
just a piece of memory and does not reference other resoruces, so the
destructing it late does not matter (unlike registration). So at least it is
*possible* to close the hole.
> (The fact that we did accept this for coherent allocations is also one reason
> why I was recently arguing that we can also make the forget() issue an accepted
> soundness hole for registrations.)
>
For registration because it references other resources so I think we cannot
close the hole with changes internal to each subsystem.
Best,
Gary
> [1] https://lore.kernel.org/all/6a7910da.9c11d2ce.289b96.00da.GAE@google.com/
>
> @@ -588,26 +587,20 @@ fn from(value: CoherentBox<T>) -> Self {
> /// to an allocated region of coherent memory and `dma_addr` is the DMA address base of the
> /// region.
> /// - The size in bytes of the allocation is equal to size information via pointer.
> -// TODO
> //
> -// DMA allocations potentially carry device resources (e.g.IOMMU mappings), hence for soundness
> -// reasons DMA allocation would need to be embedded in a `Devres` container, in order to ensure
> -// that device resources can never survive device unbind.
> -//
> -// However, it is neither desirable nor necessary to protect the allocated memory of the DMA
> -// allocation from surviving device unbind; it would require RCU read side critical sections to
> -// access the memory, which may require subsequent unnecessary copies.
> -//
> -// Hence, find a way to revoke the device resources of a `Coherent`, but not the
> -// entire `Coherent` including the allocated memory itself.
> -pub struct Coherent<T: KnownSize + ?Sized> {
> - dev: ARef<device::Device>,
> +// The lifetime parameter ties DMA allocations to the device's bound scope, ensuring they are freed
> +// before the device is unbound under normal circumstances. However, if a `Coherent` is leaked (e.g.
> +// via `mem::forget`), device resources such as IOMMU mappings will not be released. Making all
> +// constructors `unsafe` to prevent this is considered too restrictive for the common case; this
> +// soundness hole is accepted for now.
> +pub struct Coherent<'a, T: KnownSize + ?Sized> {
> + dev: &'a device::Device<Bound>,
> dma_addr: DmaAddress,
> cpu_addr: NonNull<T>,
> dma_attrs: Attrs,
> }
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2026-09-03 15:42 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-30 19:37 [PATCH 0/4] rust: dma: tie DMA allocations to the device's bound lifetime Danilo Krummrich
2026-08-30 19:37 ` [PATCH 1/4] rust: debugfs: drop 'static bound from ScopedDir file creation methods Danilo Krummrich
2026-08-30 19:53 ` sashiko-bot
2026-09-03 13:12 ` Gary Guo
2026-09-03 15:07 ` Danilo Krummrich
2026-09-03 15:16 ` Gary Guo
2026-08-30 19:37 ` [PATCH 2/4] rust: dma: tie CoherentHandle to the device's bound lifetime Danilo Krummrich
2026-09-03 13:12 ` Gary Guo
2026-08-30 19:37 ` [PATCH 3/4] samples: rust_dma: separate driver type from driver data Danilo Krummrich
2026-08-30 19:57 ` sashiko-bot
2026-09-03 13:13 ` Gary Guo
2026-08-30 19:37 ` [PATCH 4/4] rust: dma: tie Coherent and CoherentBox to the device's bound lifetime Danilo Krummrich
2026-08-30 19:47 ` sashiko-bot
2026-09-03 13:20 ` Gary Guo
2026-09-03 15:22 ` Danilo Krummrich
2026-09-03 15:42 ` Gary Guo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox