Message-ID: <2025090850-canon-banish-baf6@gregkh>
Date: Mon, 8 Sep 2025 16:16:52 +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 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.
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...

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?

> You don't want a a field to be reference counted just because it's exposed via
> debugfs.

Exactly, the data is the thing driving this, not the debugfs file.

> >> Instead you would change your struct Foo to just be:
> >> 
> >> 	struct Foo {
> >> 	   a: File<u32>,
> >> 	   b: File<u32>,
> >> 	}
> >> 
> >> If you now create an instance of Foo (let's call it `foo`), then foo.a or foo.b
> >> dereferences to the inner type, i.e. the u32. Or in other words `foo` still
> >> behaves as if `a` and `b` would be u32 values. For instance:
> >> 
> >>    if foo.a == 42 {
> >>       pr_info!("Foo::b = {}\n", foo.b);
> >>    }
> >
> > Oh that's not going to work well at all :(
> >
> > Think about something "simple" like a pci config descriptor.  You have a
> > structure, with fields, already sitting there.  You want to expose those
> > fields in debugfs.
> 
> This is more of a special case that is addressed by the Scope API in patch 6 and
> patch 7, so we should be good.

See above for my lack of understanding of that :)

> > And what happens if debugfs is not enabled?  What about if creating the
> > file fails?  The variable still needs to be present and active and
> > working.
> 
> This is the case, the variable will still be present and active in any case.

Ugh, but really, that's very unworkable overall.  While I see the logic
here, it's making the debugfs interface be the "main" one, when really
that is just an afterthought and is NOT the thing to focus on at all.

Again, debugfs is just "on the side for debugging", let's not force it
to be the way that data is accessed within the kernel itself, like is
being done with the wrapping of File<T> here.

thanks,

greg k-h

Powered by blists - more mailing lists