[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <DAABXF5QDYF0.21V01UJODPM89@kernel.org>
Date: Sat, 31 May 2025 14:03:05 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Danilo Krummrich" <dakr@...nel.org>
Cc: <gregkh@...uxfoundation.org>, <rafael@...nel.org>, <ojeda@...nel.org>,
<alex.gaynor@...il.com>, <boqun.feng@...il.com>, <gary@...yguo.net>,
<bjorn3_gh@...tonmail.com>, <benno.lossin@...ton.me>,
<a.hindborg@...nel.org>, <aliceryhl@...gle.com>, <tmgross@...ch.edu>,
<chrisi.schrefl@...il.com>, <rust-for-linux@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 7/7] rust: sample: misc: implement device driver sample
On Sat May 31, 2025 at 12:29 PM CEST, Danilo Krummrich wrote:
> On Sat, May 31, 2025 at 10:11:08AM +0200, Benno Lossin wrote:
>> On Sat May 31, 2025 at 12:24 AM CEST, Danilo Krummrich wrote:
>> > On Fri, May 30, 2025 at 10:15:37PM +0200, Benno Lossin wrote:
>> >> On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
>> >> > +config SAMPLE_RUST_MISC_DEVICE_WITH_PARENT
>> >> > + bool "Create a misc device with a parent device"
>> >> > + depends on SAMPLE_RUST_MISC_DEVICE
>> >> > + default n
>> >> > + help
>> >> > + Say Y here if you want the misc device sample to create a misc
>> >> > + device with a parent device.
>> >> > +
>> >>
>> >> Why not create a separate file? The `cfg`s might confuse newcomers
>> >> looking at the sample.
>> >
>> > It would be a lot of duplicated code, unless we really *only* exercise the
>> > device creation and registration part, which would be a bit unfortunate, given
>> > that this sample is also a pretty good test.
>>
>> We could separate the common parts into a single file and then
>> `include!` that file from the two samples. (Or if the build system
>> supports multi-file samples then just use that, but my gut feeling is
>> that it doesn't)
>
> The samples are normal modules, where we can have multiple files. But I don't
> see how that helps.
>
> `include!` works, but I'm not sure it's that much better.
>
> Another option would be to put the `cfg` on the module!() macro itself and have
> two separate module types, this way there is only a `cfg` on the two module!()
> invocations.
How about we do it like this:
We create samples/rust/rust_misc_device/{module.rs,parent.rs,common.rs}
and `module.rs`/`parent.rs` are the two entry points. Both of these
files:
* include `common.rs` using `include!` at the very top.
* define a `RustMiscDeviceModule` struct and implmement `InPlaceModule`
for it.
The module-level docs, common imports constants, `module!` invocation &
other definitions stay in `common.rs`.
This way we can build them at the same time and have no cfgs :)
>> >> > @@ -205,6 +222,31 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
>> >> > }
>> >> > }
>> >> >
>> >> > +#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
>> >> > +impl kernel::Module for RustMiscDeviceModule {
>> >> > + fn init(_module: &'static ThisModule) -> Result<Self> {
>> >> > + let options = Self::init();
>> >> > + let faux = faux::Registration::new(c_str!("rust-misc-device-sample"), None)?;
>> >> > +
>> >> > + // For every other bus, this would be called from Driver::probe(), which would return a
>>
>> Missing '`' around Driver::probe().
>>
>> >> > + // `Result<Pin<KBox<T>>>`, but faux always binds to a "dummy" driver, hence probe() is
>> >>
>> >> Not clear what `T` is supposed to be, do you mean `Self`?
>> >
>> > From the perspective of the type implementing the corresponding Driver trait it
>> > would indeed be `Self`. But I found it ambiguous to write `Self`, since I do *not*
>> > mean `RustMiscDeviceModule` with `Self`.
>>
>> Yeah that makes sense, I already entered into the `impl Driver` context
>> :) How about we use `<T as Driver>::probe()` above and then `T` makes
>> sense?
>
> Yep, that sounds good.
>
>> Another thing: faux devices don't have a `probe` in rust, so saying "not
>> required" doesn't make much sense, right?
>
> In Rust, faux does not have probe() indeed, but that's because it's "not
> required"; I can't think of a use-case for a new driver (yet), where this isn't
> just unnecessary overhead.
I'd write something along the lines of "the faux rust abstractions do
not have a `probe`, since it's unnecessary, so we initialize the
registration here".
---
Cheers,
Benno
Powered by blists - more mailing lists