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 <[email protected]> > 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 <[email protected]>
Reviewed-by: Gary Guo <[email protected]> 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`.
