Message-ID: <2025090808-slicer-consent-6db0@gregkh>
Date: Mon, 8 Sep 2025 18:19:20 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Matthew Maurer <mmaurer@...gle.com>, Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Sami Tolvanen <samitolvanen@...gle.com>,
	Timur Tabi <ttabi@...dia.com>, Benno Lossin <lossin@...nel.org>,
	Dirk Beheme <dirk.behme@...bosch.com>, linux-kernel@...r.kernel.org,
	rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v11 2/7] rust: debugfs: Add support for read-only files

On Mon, Sep 08, 2025 at 04:59:16PM +0200, Danilo Krummrich wrote:
> On Mon Sep 8, 2025 at 4:16 PM CEST, Greg Kroah-Hartman wrote:
> > On Mon, Sep 08, 2025 at 03:36:46PM +0200, Danilo Krummrich wrote:
> >> On Mon Sep 8, 2025 at 3:30 PM CEST, Greg Kroah-Hartman wrote:
> >> > On Mon, Sep 08, 2025 at 03:22:41PM +0200, Danilo Krummrich wrote:
> >> >> On Mon Sep 8, 2025 at 2:48 PM CEST, Greg Kroah-Hartman wrote:
> >> >> > On Mon, Sep 08, 2025 at 12:54:46PM +0200, Danilo Krummrich wrote:
> >> >> >> diff --git a/samples/rust/rust_debugfs.rs b/samples/rust/rust_debugfs.rs
> >> >> >> index b26eea3ee723..475502f30b1a 100644
> >> >> >> --- a/samples/rust/rust_debugfs.rs
> >> >> >> +++ b/samples/rust/rust_debugfs.rs
> >> >> >> @@ -59,6 +59,8 @@ struct RustDebugFs {
> >> >> >>      #[pin]
> >> >> >>      _compatible: File<CString>,
> >> >> >>      #[pin]
> >> >> >> +    _test: File<&'static CStr>,
> >> >> >> +    #[pin]
> >> >> >>      counter: File<AtomicUsize>,
> >> >> >>      #[pin]
> >> >> >>      inner: File<Mutex<Inner>>,
> >> >> >> @@ -140,6 +142,7 @@ fn new(pdev: &platform::Device<Core>) -> impl PinInit<Self, Error> + '_ {
> >> >> >>                          .property_read::<CString>(c_str!("compatible"))
> >> >> >>                          .required_by(dev)?,
> >> >> >>                  ),
> >> >> >> +                _test <- debugfs.read_only_file(c_str!("test"), c_str!("some_value")),
> >> >> >
> >> >> > Cool, but again, we do not want to ever be storing individual debugfs
> >> >> > files.  Well, we can, but for 90% of the cases, we do not, we only want
> >> >> > to remove the whole directory when that goes out of scope, which will
> >> >> > clean up the files then.
> >> >> 
> >> >> This API does not work in the way that you have a struct storing the data you
> >> >> want to expose *and* another one for the files with the data attached.
> >> >> 
> >> >> The File type contains the actual data. For instance, if you have a struct Foo,
> >> >> where you want to expose the members through debugfs you would *not* do:
> >> >> 
> >> >> 	struct Foo {
> >> >> 	   a: u32,
> >> >> 	   b: u32,
> >> >> 	}
> >> >> 
> >> >> 	struct FooFiles {
> >> >> 	   a: File<&u32>,
> >> >> 	   b: File<&u32>
> >> >> 	}
> >> >> 
> >> >> and then create an instance of Foo *and* another instance of FooFiles to export
> >> >> them via debugfs.
> >> >
> >> > Ah, that's exactly what I was trying to do.
> >> 
> >> But that's bad, then we're back at the lifetime problem from the beginning,
> >> because the File<&Foo> then somehow needs to ensure that the instance Foo
> >> remains alive as long as File<&Foo> or the backing directory exists.
> >> 
> >> So, you eventually end of with Foo needing to be reference counted with its own
> >> memory allocation, which horribly messes with your lifetimes in the driver.
> >
> > Once I want to drop Foo, FooFiles should "go out of scope" and be gone.
> 
> We agree on the goal here, but unfortunately it's not really possible. There are
> two options that were already exercised:
> 
> 	(1) Force that FooFiles (or FooDir) is bound to the lifetime of a
> 	    reference of Foo with FooDir<&'a Foo>.
> 
> 	    This isn't workable because we then can't store both of them into
> 	    the same parent structure.
> 
> 	(2) Reference count Foo (Arc<Foo>) and make FooDir own a referenc count
> 	    of Foo.
> 
> 	    But this is bad for the mentioned reasons. :(
> 
> 	(3) The File<T> API we have now, which gives you the behavior you ask
> 	    for with Scope<T>.
> 
> 	    Where Scope<T> creates a directory and owns the data you pass to it,
> 	    e.g. a pci config descriptor.
> 
> 	    The user can create an arbitrary number of files exporting any of
> 	    the fields in date that live in the scope and don't need to be tracked
> 	    separately, i.e. don't create separate object instances.
> 
> 	    The directory (and hence all the files) is removed once the Scope<T>
> 	    is dropped, including the data it owns.
> 
> > If a backing file descriptor is still held open, it will then become
> > "stale" and not work.  Much like the revokable stuff works.
> >
> > Note, none of this is in the C code today, and debugfs is bound to root
> > permissions, so it's not really an issue, but I can understand the goal
> > of correctness...
> 
> The lifetime guarantee we talk about is about the debugfs file still having a
> pointer to data that has already been dropped / freed.
> 
> In C you have to remove the debugfs file or directly (and hence the file) before
> the data exposed through it is freed. In C this is on the driver to take care
> of.
> 
> (If in C a driver has multiple structures exported in the same debugfs directory
> it has to manually take care of keeping all structures alive as long as the
> directory (and hence all files) exist.)
> 
> In Rust we need the abstraction to guarantee this.
> 
> > Anyway, I looked at the scoped example here, and I don't see how that
> > works any differently.  How can I use it to have a single Dir "handle"
> > that when goes out of scope, can drop the files attached to it that were
> > created to reference Foo.a and Foo.b in your example above?
> 
> In the example above you would move Foo into the Scope<Foo>. For instance:
> 
> 	let dir = root_dir.scope(foo, cstr!("subdir"), |foo, dir| {
> 		dir.read_only_file(c_str!("a"), foo.a);
> 		dir.read_only_file(c_str!("b"), foo.b);
> 	});
> 
> Note that those methods don't return anything, they're automatically bound to
> the Scope in lifetime.
> 
> So, Foo could be your pci config descriptor.
> 
> If `dir` is dropped, everything dies, the Scope, the "subdir" directory, all the
> files and also Foo.
> 
> I can provide some working code later on (currently in a meeting). :)

Working code for the simple "foo" example will be good.  Here's my
horrible (and will not build) example I was trying to get to work.

thanks,

greg k-h

Download attachment "rust_debugfs2.rs" of type "application/rls-services+xml" (1854 bytes)

Powered by blists - more mailing lists