This series provides safe DebugFS bindings for Rust, with a sample
module using them.

Example interaction with the sample driver:
~ # mount -t debugfs debugfs /debug
~ # cat /debug/sample_debugfs/*
"sample-debugfs"
0
0
Inner { x: 3, y: 10 }
~ # cat /debug/sample_debugfs/inc_counter
1
~ # cat /debug/sample_debugfs/inc_counter
2
~ # cat /debug/sample_debugfs/inc_counter
3
~ # cat /debug/sample_debugfs/*
"sample-debugfs"
4
4
Inner { x: 3, y: 10 }
~ # cat /debug/sample_control/swap
Swapped!
~ # cat /debug/sample_debugfs/*
"sample-debugfs"
5
5
Inner { x: 10, y: 3 }
~ # cat /debug/sample_control/add_counter 
Counter added!
~ # cat /debug/sample_debugfs/*
"sample-debugfs"
6
6
Inner { x: 16, y: 3 }
~ # 

Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
Changes in v9:
- Switched to `PinInit` backing instead of `ForeignOwnable`
- Changed sample to be a platform driver
- Exported a static property
- Demonstrated runtime mutation in platform driver (`inc_counter`)
- Demonstrated how driver code would interact with data structures
  exported through DebugFS (`Wrapper`)
- Link to v8: https://lore.kernel.org/r/20250627-debugfs-rust-v8-0-c6526e413d40@google.com

Changes in v8:
- Switched from casts to `core::from_{ref, mut}` in type change
- Link to v7: https://lore.kernel.org/r/20250624-debugfs-rust-v7-0-9c8835a7a20f@google.com

Changes in v7:
- Rewrote `entry::Entry` -> `Entry`
- Use `c_int` and `c_void` from kernel prelude rather than core
- Removed unnecessary `display_open` cast
- Switched from `Deref` + an explicit box to `ForeignOwnable` for
  attaching owned data.
- Made `&'static` and `&'static mut` implement `ForeignOwnable`
- Swapped "driver" to "module" in sample code
- Link to v6: https://lore.kernel.org/r/20250618-debugfs-rust-v6-0-72cae211b133@google.com

Changes in v6:
- Replaced explicit lifetimes with children keeping their parents alive.
- Added support for attaching owned data.
- Removed recomendation to only keep root handles and handles you want
  to delete around.
- Refactored some code into separate files to improve clarity.
- Link to v5: https://lore.kernel.org/r/20250505-debugfs-rust-v5-0-3e93ce7bb76e@google.com

Changes in v5:
- Made Dir + File wrappers around Entry
- All functions return owning handles. To discard without drop, use
  `forget`. To keep a handle without drop, use `ManuallyDrop`.
- Fixed bugs around `not(CONFIG_DEBUG_FS)`
- Removed unnecessary `addr_of!`
- Link to v4: https://lore.kernel.org/r/20250502-debugfs-rust-v4-0-788a9c6c2e77@google.com

Changes in v4:
- Remove SubDir, replace with type-level constant.
- Add lifetime to Dir to prevent subdirectories and files from outliving
  their parents and triggering an Oops when accessed.
- Split unsafe blocks with two calls into two blocks
- Access `private` field through direct pointer dereference, avoiding
  creation of a reference to it.
- Notably not changed - owning/non-owning handle defaults. The best read
  I had from the thread was to continue with this mode, but I'm willing
  to change if need be.
- Comment changes
  - More comment markdown
  - Remove scopes from examples
  - Put `as_ptr` properties into a `# Guarantees` section.
- Link to v3: https://lore.kernel.org/r/20250501-debugfs-rust-v3-0-850869fab672@google.com

Changes in v3:
- Split `Dir` into `Dir`/`SubDir`/`File` to improve API.
- Add "." to end of all comments.
- Convert INVARIANT to # Invariants on types.
- Add backticks everywhere I found variables/types in my comments.
- Promoted invariant comment to doc comment.
- Extended sample commenting to make it clearer what is happening.
- Link to v2: https://lore.kernel.org/r/20250430-debugfs-rust-v2-0-2e8d3985812b@google.com

Changes in v2:
- Drop support for builder / pinned bindings in initial series
- Remove `ARef` usage to abstract the dentry nature of handles
- Remove error handling to discourage users from caring whether DebugFS
  is enabled.
- Support CONFIG_DEBUG_FS=n while leaving the API available
- Fixed mistaken decimal/octal mixup
- Doc/comment cleanup
- Link to v1: https://lore.kernel.org/r/20250429-debugfs-rust-v1-0-6b6e7cb7929f@google.com

---
Matthew Maurer (5):
      rust: debugfs: Bind DebugFS directory creation
      rust: debugfs: Bind file creation for long-lived Display
      rust: debugfs: Support `PinInit` backing for `File`s.
      rust: debugfs: Support format hooks
      rust: samples: Add debugfs sample

 MAINTAINERS                         |   3 +
 rust/bindings/bindings_helper.h     |   1 +
 rust/kernel/debugfs.rs              | 286 ++++++++++++++++++++++++++++++++++++
 rust/kernel/debugfs/display_file.rs | 100 +++++++++++++
 rust/kernel/debugfs/entry.rs        |  66 +++++++++
 rust/kernel/lib.rs                  |   1 +
 samples/rust/Kconfig                |  11 ++
 samples/rust/Makefile               |   1 +
 samples/rust/rust_debugfs.rs        | 182 +++++++++++++++++++++++
 9 files changed, 651 insertions(+)
---
base-commit: 6d16cd5769bbb5eb62974e8eddb97fca830b49fd
change-id: 20250428-debugfs-rust-3cd5c97eb7d1

Best regards,
-- 
Matthew Maurer <mmaurer@google.com>

On Wed Jul 9, 2025 at 9:09 PM CEST, Matthew Maurer wrote:
> This series provides safe DebugFS bindings for Rust, with a sample
> module using them.
>
> Example interaction with the sample driver:

I understand what you're trying to do here, i.e. showcase that values exported
via debugfs can be altered.

The problem is that the current abstractions only implement read(), but not
write().

So, to work around this you create multiple files for a single value: one that
only prints the value on read() and one that modifies the value on read().

For instance, you have a value counter (AtomicUsize), which is exported as:

	$(DEBUGFS_ROOT)/counter
	$(DEBUGFS_ROOT)/inc_counter

where 

	$ cat $(DEBUGFS_ROOT)/counter

prints the value and

	$ cat $(DEBUGFS_ROOT)/inc_counter

increments the counter.

While this is technically not wrong it's providing bad guidance to people.

Instead this should be something along the lines of:

	$ cat $(DEBUGFS_ROOT)/counter
	0

	$ echo "++" > $(DEBUGFS_ROOT)/counter
	$ cat $(DEBUGFS_ROOT)/counter
	1

	$ echo "42" > $(DEBUGFS_ROOT)/counter
	$ cat $(DEBUGFS_ROOT)/counter
	42

Given that the abstraction doesn't support write() yet, just don't try to work
around it.

If you really want to showcase changing values, you can, for instance, create a
workqueue inside the sample driver and modify the counter periodically.

We really should not teach people to modify values by read() instead of write().
Also, without this workaround there shouldn't be a reason to export the exact
same value twice, i.e. no need for File<File<AtomicUsize>>.

- Danilo

On Wed, Jul 9, 2025 at 2:47 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Wed Jul 9, 2025 at 9:09 PM CEST, Matthew Maurer wrote:
> > This series provides safe DebugFS bindings for Rust, with a sample
> > module using them.
> >
> > Example interaction with the sample driver:
>
> I understand what you're trying to do here, i.e. showcase that values exported
> via debugfs can be altered.
>
> The problem is that the current abstractions only implement read(), but not
> write().

I was trying to keep the initial bindings simple. Adding `write` is
definitely something we could do, but I thought maybe that could be in
a subsequent patch.

> If you really want to showcase changing values, you can, for instance, create a
> workqueue inside the sample driver and modify the counter periodically.

This is supposed to be sample code, so ideally it should be as narrow
as is reasonable in what subsystems it touches, no? If people would
really prefer the sample schedule a ticking counter I can do that, but
it already felt weird to be registering a platform driver in a debugfs
sample.

>
> We really should not teach people to modify values by read() instead of write().
> Also, without this workaround there shouldn't be a reason to export the exact
> same value twice, i.e. no need for File<File<AtomicUsize>>.
>
> - Danilo

How do you feel about the `Wrapper` struct, intended to simulate the
driver doing its actual job and show how that would look? Is that
similarly verboten, even though there's a comment on it saying this
isn't how one should do things?

On Wed Jul 9, 2025 at 11:53 PM CEST, Matthew Maurer wrote:
> On Wed, Jul 9, 2025 at 2:47 PM Danilo Krummrich <dakr@kernel.org> wrote:
>>
>> On Wed Jul 9, 2025 at 9:09 PM CEST, Matthew Maurer wrote:
>> > This series provides safe DebugFS bindings for Rust, with a sample
>> > module using them.
>> >
>> > Example interaction with the sample driver:
>>
>> I understand what you're trying to do here, i.e. showcase that values exported
>> via debugfs can be altered.
>>
>> The problem is that the current abstractions only implement read(), but not
>> write().
>
> I was trying to keep the initial bindings simple. Adding `write` is
> definitely something we could do, but I thought maybe that could be in
> a subsequent patch.

Absolutely, yes! I didn't mean to ask to add it now. :)

>> If you really want to showcase changing values, you can, for instance, create a
>> workqueue inside the sample driver and modify the counter periodically.
>
> This is supposed to be sample code, so ideally it should be as narrow
> as is reasonable in what subsystems it touches, no? If people would
> really prefer the sample schedule a ticking counter I can do that, but
> it already felt weird to be registering a platform driver in a debugfs
> sample.

I'm not asking to do that. If the values don't change for now, because
there's no write() yet, that's perfectly fine with me. :)

>>
>> We really should not teach people to modify values by read() instead of write().
>> Also, without this workaround there shouldn't be a reason to export the exact
>> same value twice, i.e. no need for File<File<AtomicUsize>>.
>>
>> - Danilo
>
> How do you feel about the `Wrapper` struct, intended to simulate the
> driver doing its actual job and show how that would look? Is that
> similarly verboten, even though there's a comment on it saying this
> isn't how one should do things?

Yeah, let's not do that -- don't give people ideas. :)

On Wed, Jul 9, 2025 at 2:59 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Wed Jul 9, 2025 at 11:53 PM CEST, Matthew Maurer wrote:
> > On Wed, Jul 9, 2025 at 2:47 PM Danilo Krummrich <dakr@kernel.org> wrote:
> >>
> >> On Wed Jul 9, 2025 at 9:09 PM CEST, Matthew Maurer wrote:
> >> > This series provides safe DebugFS bindings for Rust, with a sample
> >> > module using them.
> >> >
> >> > Example interaction with the sample driver:
> >>
> >> I understand what you're trying to do here, i.e. showcase that values exported
> >> via debugfs can be altered.
> >>
> >> The problem is that the current abstractions only implement read(), but not
> >> write().
> >
> > I was trying to keep the initial bindings simple. Adding `write` is
> > definitely something we could do, but I thought maybe that could be in
> > a subsequent patch.
>
> Absolutely, yes! I didn't mean to ask to add it now. :)
>
> >> If you really want to showcase changing values, you can, for instance, create a
> >> workqueue inside the sample driver and modify the counter periodically.
> >
> > This is supposed to be sample code, so ideally it should be as narrow
> > as is reasonable in what subsystems it touches, no? If people would
> > really prefer the sample schedule a ticking counter I can do that, but
> > it already felt weird to be registering a platform driver in a debugfs
> > sample.
>
> I'm not asking to do that. If the values don't change for now, because
> there's no write() yet, that's perfectly fine with me. :)

Potentially I misinterpreted Greg[1], I thought he wanted to see how
mutation would work.
If we don't need mutation, I'm fine simplifying the driver to not have
any mutation triggers and just export a few random things.

[1]: https://lore.kernel.org/all/2025070349-tricky-arguable-5362@gregkh/

>
> >>
> >> We really should not teach people to modify values by read() instead of write().
> >> Also, without this workaround there shouldn't be a reason to export the exact
> >> same value twice, i.e. no need for File<File<AtomicUsize>>.
> >>
> >> - Danilo
> >
> > How do you feel about the `Wrapper` struct, intended to simulate the
> > driver doing its actual job and show how that would look? Is that
> > similarly verboten, even though there's a comment on it saying this
> > isn't how one should do things?
>
> Yeah, let's not do that -- don't give people ideas. :)

On Wed, Jul 09, 2025 at 03:04:51PM -0700, Matthew Maurer wrote:
> On Wed, Jul 9, 2025 at 2:59 PM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Wed Jul 9, 2025 at 11:53 PM CEST, Matthew Maurer wrote:
> > > On Wed, Jul 9, 2025 at 2:47 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > >>
> > >> On Wed Jul 9, 2025 at 9:09 PM CEST, Matthew Maurer wrote:
> > >> > This series provides safe DebugFS bindings for Rust, with a sample
> > >> > module using them.
> > >> >
> > >> > Example interaction with the sample driver:
> > >>
> > >> I understand what you're trying to do here, i.e. showcase that values exported
> > >> via debugfs can be altered.
> > >>
> > >> The problem is that the current abstractions only implement read(), but not
> > >> write().
> > >
> > > I was trying to keep the initial bindings simple. Adding `write` is
> > > definitely something we could do, but I thought maybe that could be in
> > > a subsequent patch.
> >
> > Absolutely, yes! I didn't mean to ask to add it now. :)
> >
> > >> If you really want to showcase changing values, you can, for instance, create a
> > >> workqueue inside the sample driver and modify the counter periodically.
> > >
> > > This is supposed to be sample code, so ideally it should be as narrow
> > > as is reasonable in what subsystems it touches, no? If people would
> > > really prefer the sample schedule a ticking counter I can do that, but
> > > it already felt weird to be registering a platform driver in a debugfs
> > > sample.
> >
> > I'm not asking to do that. If the values don't change for now, because
> > there's no write() yet, that's perfectly fine with me. :)
> 
> Potentially I misinterpreted Greg[1], I thought he wanted to see how
> mutation would work.
> If we don't need mutation, I'm fine simplifying the driver to not have
> any mutation triggers and just export a few random things.

I mean, the most simple way would be to create the debugfs entries in probe()
and mutate them - still in probe() - right afterwards once. I think we should
do in any case. And AFAICT, this also covers [1].

> [1]: https://lore.kernel.org/all/2025070349-tricky-arguable-5362@gregkh/
> 
> >
> > >>
> > >> We really should not teach people to modify values by read() instead of write().
> > >> Also, without this workaround there shouldn't be a reason to export the exact
> > >> same value twice, i.e. no need for File<File<AtomicUsize>>.
> > >>
> > >> - Danilo
> > >
> > > How do you feel about the `Wrapper` struct, intended to simulate the
> > > driver doing its actual job and show how that would look? Is that
> > > similarly verboten, even though there's a comment on it saying this
> > > isn't how one should do things?
> >
> > Yeah, let's not do that -- don't give people ideas. :)

On Thu, Jul 10, 2025 at 12:12:15AM +0200, Danilo Krummrich wrote:
> On Wed, Jul 09, 2025 at 03:04:51PM -0700, Matthew Maurer wrote:
> > On Wed, Jul 9, 2025 at 2:59 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > >
> > > On Wed Jul 9, 2025 at 11:53 PM CEST, Matthew Maurer wrote:
> > > > On Wed, Jul 9, 2025 at 2:47 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > > >>
> > > >> On Wed Jul 9, 2025 at 9:09 PM CEST, Matthew Maurer wrote:
> > > >> > This series provides safe DebugFS bindings for Rust, with a sample
> > > >> > module using them.
> > > >> >
> > > >> > Example interaction with the sample driver:
> > > >>
> > > >> I understand what you're trying to do here, i.e. showcase that values exported
> > > >> via debugfs can be altered.
> > > >>
> > > >> The problem is that the current abstractions only implement read(), but not
> > > >> write().
> > > >
> > > > I was trying to keep the initial bindings simple. Adding `write` is
> > > > definitely something we could do, but I thought maybe that could be in
> > > > a subsequent patch.
> > >
> > > Absolutely, yes! I didn't mean to ask to add it now. :)
> > >
> > > >> If you really want to showcase changing values, you can, for instance, create a
> > > >> workqueue inside the sample driver and modify the counter periodically.
> > > >
> > > > This is supposed to be sample code, so ideally it should be as narrow
> > > > as is reasonable in what subsystems it touches, no? If people would
> > > > really prefer the sample schedule a ticking counter I can do that, but
> > > > it already felt weird to be registering a platform driver in a debugfs
> > > > sample.
> > >
> > > I'm not asking to do that. If the values don't change for now, because
> > > there's no write() yet, that's perfectly fine with me. :)
> > 
> > Potentially I misinterpreted Greg[1], I thought he wanted to see how
> > mutation would work.
> > If we don't need mutation, I'm fine simplifying the driver to not have
> > any mutation triggers and just export a few random things.
> 
> I mean, the most simple way would be to create the debugfs entries in probe()
> and mutate them - still in probe() - right afterwards once. I think we should
> do in any case. And AFAICT, this also covers [1].
> 
> > [1]: https://lore.kernel.org/all/2025070349-tricky-arguable-5362@gregkh/

Ugh.

Yes we need write.  And read, and custom file-ops, and the like as
that's what debugfs is doing today for C code!  We need this to be as
simple as, or almost as simple as, what we have today in C or no one is
going to use this stuff and go off and attempt to write their own mess.

While I would love to have something as simple as:
	void debugfs_create_u8(const char *name, umode_t mode, struct dentry *parent, u8 *value);
like we do today.  I understand that this makes all sorts of
"assumptions" that Rust really doesn't like (i.e. lifetime of *value and
the like), BUT we MUST have something like this for Rust users, as
that's going to ensure that people actually use this api.

Look at an in-kernel function today, like ath9k_init_debug() that
creates a metric-ton of debugfs files and binds them to different
variables that are owned by a structure and more complex data structures
and memory dumps and other random file interactions.  We need, in Rust,
a way to do everything that that function can do today, in a SIMPLE
manner that reads just as easily as ath9k_init_debug() does.

So no "we will add write support later" stuff, sorry, real drivers
require write support in debugfs.

thanks,

greg k-h

On Thu Jul 10, 2025 at 7:27 AM CEST, Greg Kroah-Hartman wrote:
> Ugh.
>
> Yes we need write.  And read, and custom file-ops, and the like as
> that's what debugfs is doing today for C code!  We need this to be as
> simple as, or almost as simple as, what we have today in C or no one is
> going to use this stuff and go off and attempt to write their own mess.

I agree, we really want the helpers you're referring to below. I think we
discussed this in previous iterations already.

> While I would love to have something as simple as:
> 	void debugfs_create_u8(const char *name, umode_t mode, struct dentry *parent, u8 *value);
> like we do today.  I understand that this makes all sorts of
> "assumptions" that Rust really doesn't like (i.e. lifetime of *value and
> the like), BUT we MUST have something like this for Rust users, as
> that's going to ensure that people actually use this api.

I think it can be as simple as

	void debugfs_create_u8(const char *name, umode_t mode, struct dentry *parent, u8 *value);

in Rust as well. Declaring this in a structure looks like this.

	struct Data {
	   counter: File<u8>,
	}

Given that we have some Dir instance, this can be as simple as:

	dir.create_file_u8(...);

Which uses default callbacks for read(), write(), etc.

> Look at an in-kernel function today, like ath9k_init_debug() that
> creates a metric-ton of debugfs files and binds them to different
> variables that are owned by a structure and more complex data structures
> and memory dumps and other random file interactions.  We need, in Rust,
> a way to do everything that that function can do today, in a SIMPLE
> manner that reads just as easily as ath9k_init_debug() does.

That's possible with the current design and code, it misses the helpers, such as
create_file_u8() above, to reduce the boilerplate though. With that, it should
look pretty similar.

> So no "we will add write support later" stuff, sorry, real drivers
> require write support in debugfs.

Adding the write callback seems rather simple, so it should also be fine to add
it right away.

From a design point of view the things above basically come down to different
variants of create_file().

So, it should mostly be sufficient to add subsequent patches to this series
implementing those.

On Thu Jul 10, 2025 at 11:36 AM CEST, Danilo Krummrich wrote:
> On Thu Jul 10, 2025 at 7:27 AM CEST, Greg Kroah-Hartman wrote:
>> Ugh.
>>
>> Yes we need write.  And read, and custom file-ops, and the like as
>> that's what debugfs is doing today for C code!  We need this to be as
>> simple as, or almost as simple as, what we have today in C or no one is
>> going to use this stuff and go off and attempt to write their own mess.
>
> I agree, we really want the helpers you're referring to below. I think we
> discussed this in previous iterations already.
>
>> While I would love to have something as simple as:
>> 	void debugfs_create_u8(const char *name, umode_t mode, struct dentry *parent, u8 *value);
>> like we do today.  I understand that this makes all sorts of
>> "assumptions" that Rust really doesn't like (i.e. lifetime of *value and
>> the like), BUT we MUST have something like this for Rust users, as
>> that's going to ensure that people actually use this api.
>
> I think it can be as simple as
>
> 	void debugfs_create_u8(const char *name, umode_t mode, struct dentry *parent, u8 *value);
>
> in Rust as well. Declaring this in a structure looks like this.
>
> 	struct Data {
> 	   counter: File<u8>,
> 	}
>
> Given that we have some Dir instance, this can be as simple as:
>
> 	dir.create_file_u8(...);
>
> Which uses default callbacks for read(), write(), etc.
>
>> Look at an in-kernel function today, like ath9k_init_debug() that
>> creates a metric-ton of debugfs files and binds them to different
>> variables that are owned by a structure and more complex data structures
>> and memory dumps and other random file interactions.  We need, in Rust,
>> a way to do everything that that function can do today, in a SIMPLE
>> manner that reads just as easily as ath9k_init_debug() does.
>
> That's possible with the current design and code, it misses the helpers, such as
> create_file_u8() above, to reduce the boilerplate though. With that, it should
> look pretty similar.

Can't you just implement the traits directly on `u8` and then just call
`create_file`?

---
Cheers,
Benno

On Thu Jul 10, 2025 at 1:09 PM CEST, Benno Lossin wrote:
> On Thu Jul 10, 2025 at 11:36 AM CEST, Danilo Krummrich wrote:
>> On Thu Jul 10, 2025 at 7:27 AM CEST, Greg Kroah-Hartman wrote:
>>> Ugh.
>>>
>>> Yes we need write.  And read, and custom file-ops, and the like as
>>> that's what debugfs is doing today for C code!  We need this to be as
>>> simple as, or almost as simple as, what we have today in C or no one is
>>> going to use this stuff and go off and attempt to write their own mess.
>>
>> I agree, we really want the helpers you're referring to below. I think we
>> discussed this in previous iterations already.
>>
>>> While I would love to have something as simple as:
>>> 	void debugfs_create_u8(const char *name, umode_t mode, struct dentry *parent, u8 *value);
>>> like we do today.  I understand that this makes all sorts of
>>> "assumptions" that Rust really doesn't like (i.e. lifetime of *value and
>>> the like), BUT we MUST have something like this for Rust users, as
>>> that's going to ensure that people actually use this api.
>>
>> I think it can be as simple as
>>
>> 	void debugfs_create_u8(const char *name, umode_t mode, struct dentry *parent, u8 *value);
>>
>> in Rust as well. Declaring this in a structure looks like this.
>>
>> 	struct Data {
>> 	   counter: File<u8>,
>> 	}
>>
>> Given that we have some Dir instance, this can be as simple as:
>>
>> 	dir.create_file_u8(...);
>>
>> Which uses default callbacks for read(), write(), etc.
>>
>>> Look at an in-kernel function today, like ath9k_init_debug() that
>>> creates a metric-ton of debugfs files and binds them to different
>>> variables that are owned by a structure and more complex data structures
>>> and memory dumps and other random file interactions.  We need, in Rust,
>>> a way to do everything that that function can do today, in a SIMPLE
>>> manner that reads just as easily as ath9k_init_debug() does.
>>
>> That's possible with the current design and code, it misses the helpers, such as
>> create_file_u8() above, to reduce the boilerplate though. With that, it should
>> look pretty similar.
>
> Can't you just implement the traits directly on `u8` and then just call
> `create_file`?

Ah I guess for write support you need `Atomic<u8>` and that doesn't
implement `Display`...

Maybe `Display` is the wrong trait for this...

---
Cheers,
Benno

On Wed, Jul 9, 2025 at 3:12 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Wed, Jul 09, 2025 at 03:04:51PM -0700, Matthew Maurer wrote:
> > On Wed, Jul 9, 2025 at 2:59 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > >
> > > On Wed Jul 9, 2025 at 11:53 PM CEST, Matthew Maurer wrote:
> > > > On Wed, Jul 9, 2025 at 2:47 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > > >>
> > > >> On Wed Jul 9, 2025 at 9:09 PM CEST, Matthew Maurer wrote:
> > > >> > This series provides safe DebugFS bindings for Rust, with a sample
> > > >> > module using them.
> > > >> >
> > > >> > Example interaction with the sample driver:
> > > >>
> > > >> I understand what you're trying to do here, i.e. showcase that values exported
> > > >> via debugfs can be altered.
> > > >>
> > > >> The problem is that the current abstractions only implement read(), but not
> > > >> write().
> > > >
> > > > I was trying to keep the initial bindings simple. Adding `write` is
> > > > definitely something we could do, but I thought maybe that could be in
> > > > a subsequent patch.
> > >
> > > Absolutely, yes! I didn't mean to ask to add it now. :)
> > >
> > > >> If you really want to showcase changing values, you can, for instance, create a
> > > >> workqueue inside the sample driver and modify the counter periodically.
> > > >
> > > > This is supposed to be sample code, so ideally it should be as narrow
> > > > as is reasonable in what subsystems it touches, no? If people would
> > > > really prefer the sample schedule a ticking counter I can do that, but
> > > > it already felt weird to be registering a platform driver in a debugfs
> > > > sample.
> > >
> > > I'm not asking to do that. If the values don't change for now, because
> > > there's no write() yet, that's perfectly fine with me. :)
> >
> > Potentially I misinterpreted Greg[1], I thought he wanted to see how
> > mutation would work.
> > If we don't need mutation, I'm fine simplifying the driver to not have
> > any mutation triggers and just export a few random things.
>
> I mean, the most simple way would be to create the debugfs entries in probe()
> and mutate them - still in probe() - right afterwards once. I think we should
> do in any case. And AFAICT, this also covers [1].

That's what I did with my `InPlaceModule` before and it evidently
didn't count? I don't see how the constructor being `probe` rather
than `init` would have been the issue - the only change that causes is
calling `KBox::pin_init` on the value you would have returned.

>
> > [1]: https://lore.kernel.org/all/2025070349-tricky-arguable-5362@gregkh/
> >
> > >
> > > >>
> > > >> We really should not teach people to modify values by read() instead of write().
> > > >> Also, without this workaround there shouldn't be a reason to export the exact
> > > >> same value twice, i.e. no need for File<File<AtomicUsize>>.
> > > >>
> > > >> - Danilo
> > > >
> > > > How do you feel about the `Wrapper` struct, intended to simulate the
> > > > driver doing its actual job and show how that would look? Is that
> > > > similarly verboten, even though there's a comment on it saying this
> > > > isn't how one should do things?
> > >
> > > Yeah, let's not do that -- don't give people ideas. :)

On Thu Jul 10, 2025 at 12:21 AM CEST, Matthew Maurer wrote:
> On Wed, Jul 9, 2025 at 3:12 PM Danilo Krummrich <dakr@kernel.org> wrote:
>>
>> On Wed, Jul 09, 2025 at 03:04:51PM -0700, Matthew Maurer wrote:
>> > On Wed, Jul 9, 2025 at 2:59 PM Danilo Krummrich <dakr@kernel.org> wrote:
>> > >
>> > > On Wed Jul 9, 2025 at 11:53 PM CEST, Matthew Maurer wrote:
>> > > > On Wed, Jul 9, 2025 at 2:47 PM Danilo Krummrich <dakr@kernel.org> wrote:
>> > > >>
>> > > >> On Wed Jul 9, 2025 at 9:09 PM CEST, Matthew Maurer wrote:
>> > > >> > This series provides safe DebugFS bindings for Rust, with a sample
>> > > >> > module using them.
>> > > >> >
>> > > >> > Example interaction with the sample driver:
>> > > >>
>> > > >> I understand what you're trying to do here, i.e. showcase that values exported
>> > > >> via debugfs can be altered.
>> > > >>
>> > > >> The problem is that the current abstractions only implement read(), but not
>> > > >> write().
>> > > >
>> > > > I was trying to keep the initial bindings simple. Adding `write` is
>> > > > definitely something we could do, but I thought maybe that could be in
>> > > > a subsequent patch.
>> > >
>> > > Absolutely, yes! I didn't mean to ask to add it now. :)
>> > >
>> > > >> If you really want to showcase changing values, you can, for instance, create a
>> > > >> workqueue inside the sample driver and modify the counter periodically.
>> > > >
>> > > > This is supposed to be sample code, so ideally it should be as narrow
>> > > > as is reasonable in what subsystems it touches, no? If people would
>> > > > really prefer the sample schedule a ticking counter I can do that, but
>> > > > it already felt weird to be registering a platform driver in a debugfs
>> > > > sample.
>> > >
>> > > I'm not asking to do that. If the values don't change for now, because
>> > > there's no write() yet, that's perfectly fine with me. :)
>> >
>> > Potentially I misinterpreted Greg[1], I thought he wanted to see how
>> > mutation would work.
>> > If we don't need mutation, I'm fine simplifying the driver to not have
>> > any mutation triggers and just export a few random things.
>>
>> I mean, the most simple way would be to create the debugfs entries in probe()
>> and mutate them - still in probe() - right afterwards once. I think we should
>> do in any case. And AFAICT, this also covers [1].
>
> That's what I did with my `InPlaceModule` before and it evidently
> didn't count? I don't see how the constructor being `probe` rather
> than `init` would have been the issue - the only change that causes is
> calling `KBox::pin_init` on the value you would have returned.

Who said this didn't count? It makes no difference from where you mutate it, the
importent part is that you show that you can, and that is clearly covered.

The discussion you're linking to in [1] has a different context. It was about
exporting multiple values that are behind a single lock individually. And we
concluded that for the reasons mentioned in [2] it's not desirable.

[2] https://lore.kernel.org/lkml/aGZ3q0PEmZ7lV4I-@pollux/

>> > [1]: https://lore.kernel.org/all/2025070349-tricky-arguable-5362@gregkh/
>> >
>> > >
>> > > >>
>> > > >> We really should not teach people to modify values by read() instead of write().
>> > > >> Also, without this workaround there shouldn't be a reason to export the exact
>> > > >> same value twice, i.e. no need for File<File<AtomicUsize>>.
>> > > >>
>> > > >> - Danilo
>> > > >
>> > > > How do you feel about the `Wrapper` struct, intended to simulate the
>> > > > driver doing its actual job and show how that would look? Is that
>> > > > similarly verboten, even though there's a comment on it saying this
>> > > > isn't how one should do things?
>> > >
>> > > Yeah, let's not do that -- don't give people ideas. :)