Re: [PATCH v9 0/5] rust: DebugFS Bindings

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

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