Message-ID: <2025091043-mardi-splurge-f3fd@gregkh>
Date: Wed, 10 Sep 2025 17:21:34 +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 06:55:48PM +0200, Danilo Krummrich wrote:
> On Mon Sep 8, 2025 at 6:30 PM CEST, Danilo Krummrich wrote:
> > On Mon Sep 8, 2025 at 6:19 PM CEST, Greg Kroah-Hartman wrote:
> >> On Mon, Sep 08, 2025 at 04:59:16PM +0200, Danilo Krummrich wrote:
> >
> > <snip>
> >
> >>> 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.
> >
> > <snip>
> >
> >>> 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.
> >
> > Here it comes [1]. :)
> >
> > [1] rust_debugfs_soc_info.rs
> >
> > // SPDX-License-Identifier: GPL-2.0
> >
> > //! Simple `debugfs::Scope` example.
> >
> > use kernel::c_str;
> > use kernel::debugfs::{Dir, Scope};
> > use kernel::prelude::*;
> >
> > module! {
> >     type: MyModule,
> >     name: "MyModule",
> >     description: "Just a simple test module.",
> >     license: "GPL",
> > }
> >
> > #[derive(Debug)]
> > struct HwSocInfo {
> >     name: &'static CStr,
> >     ver: u32,
> >     id: u32,
> > }
> >
> > impl HwSocInfo {
> >     fn new(name: &'static CStr, ver: u32, id: u32) -> Self {
> >         Self { name, ver, id }
> >     }
> > }
> >
> > struct MyModule {
> >     // Dropped when MyModule is released (e.g. through `rmmod`).
> >     //
> >     // This will drop the inner `HwSocInfo`, the "foo" directory, and all files created within this
> >     // directory.
> >     _scope: Pin<KBox<Scope<HwSocInfo>>>,
> 
> And yes, I get that HwSocInfo now lives within a debugfs structure, just like
> with
> 
> 	struct Data {
> 	   version: File<u32>,
> 	}
> 
> but those become transparent wrappers if debugfs is disabled, i.e. zero
> overhead. They also won't make the driver fail if anything with debugfs goes
> south if enabled.
> 
> And I also understand your point that now they're part of a "real" data
> structure. But in the end, debugfs *is* part of the driver. And while we should
> ensure that it doesn't impact drivers as much as possible (which we do), I don't
> think that we necessarily have to hide the fact entirely.
> 
> Having that said, I also don't really see an alternative. If we really want
> debugfs structures to be entirely separate we would have to either
> 
>   (1) reference count fields exposed through debugfs, or
> 
>   (2) make the interface unsafe, use raw pointers and assert that a debugfs file
>       never out-lives the data it exposes, just like in C.
> 
> As I mentioned previously, while File<T> is visible in driver structures only,
> (1) even enforces drivers to break their lifetime patterns, which is much worse
> and not acceptable I think.
> 
> I would even say that (2) is better than (1), because it becomes safe when
> debugfs is disabled. Yet I think both (1) and (2) are much worse that what we
> have right now.
> 
> And I think the Scope API helps a lot in keeping things reasonable for cases
> where a lot of fields of a single structure should be exposed separately, such
> as the one you sketched up.

Thanks for the example (and the rewrite of my example.)

This makes more sense, but I'm worried that this puts debugfs "front and
center" for data structures that normally had nothing to do with debugfs
at all.

So I took at look at the USB and PCI drivers to see what they do as
those are "real-world" users of debugfs, not just "soc info" stuff, and
it turns out that you are right.  The use of debugfs is normally NOT
using just simple wrappers around variables like our examples are doing,
but are explicit "read/write" type things that do stuff to a structure,
AND those structures need to be properly referenced, so that things do
not go wrong when structures are removed.

So, after digging through this some more today, I think I'm ok with it.
Let's merge this now, and let's see how it gets used and if it's
unwieldy, hey, we can always change it!

thanks,

greg k-h

Powered by blists - more mailing lists