Re: [PATCH v11 2/7] rust: debugfs: Add support for read-only files

["Danilo Krummrich" <dakr@kernel.org>]

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). :)