Message-Id: <DCNKS0XXGU9F.1VBER18A81OYU@kernel.org>
Date: Mon, 08 Sep 2025 18:55:48 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Greg Kroah-Hartman" <gregkh@...uxfoundation.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 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.

> }
>
> impl kernel::Module for MyModule {
>     fn init(_module: &'static kernel::ThisModule) -> Result<Self, Error> {
>         let root_dir = Dir::new(c_str!("my_module"));
>
>         // Obtain some `HwSocInfo`, could from anywhere.
>         let soc_info = HwSocInfo::new(c_str!("foo"), 24, 42);
>
>         let scope = KBox::pin_init(
>             // Create directory scope, that contains some data and a bunch of files exporting this
>             // data.
>             root_dir.scope(soc_info, c_str!("hw_soc_info"), |soc_info, dir| {
>                 dir.read_only_file(c_str!("name"), &soc_info.name);
>                 dir.read_only_file(c_str!("ver"), &soc_info.ver);
>                 dir.read_only_file(c_str!("id"), &soc_info.id);
>             }),
>             GFP_KERNEL,
>         )?;
>
>         // Print the contents of `soc_info` that were moved into `scope`.
>         pr_info!("HwSocInfo: {:?}\n", &**scope);
>
>         Ok(Self { _scope: scope })
>     }
> }

Powered by blists - more mailing lists