In order to demonstrate and test a MiscDeviceRegistration with a parent
device, introduce CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT.
If CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT=y the misc device sample
is initialized with a parent device (faux), otherwise it is initialized
without a parent device, i.e. the exact same way as without this patch.
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
samples/rust/Kconfig | 8 +++++
samples/rust/rust_misc_device.rs | 50 +++++++++++++++++++++++++++++---
2 files changed, 54 insertions(+), 4 deletions(-)
diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
index b1006ab4bc3c..9948ec0939ef 100644
--- a/samples/rust/Kconfig
+++ b/samples/rust/Kconfig
@@ -30,6 +30,14 @@ config SAMPLE_RUST_MISC_DEVICE
If unsure, say N.
+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.
+
config SAMPLE_RUST_PRINT
tristate "Printing macros"
help
diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
index 9bf1a0f64e6e..175638d6d341 100644
--- a/samples/rust/rust_misc_device.rs
+++ b/samples/rust/rust_misc_device.rs
@@ -167,6 +167,9 @@
uaccess::{UserSlice, UserSliceReader, UserSliceWriter},
};
+#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
+use kernel::faux;
+
const RUST_MISC_DEV_HELLO: u32 = _IO('|' as u32, 0x80);
const RUST_MISC_DEV_GET_VALUE: u32 = _IOR::<i32>('|' as u32, 0x81);
const RUST_MISC_DEV_SET_VALUE: u32 = _IOW::<i32>('|' as u32, 0x82);
@@ -181,19 +184,33 @@
license: "GPL",
}
+#[cfg(not(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT))]
#[pin_data]
struct RustMiscDeviceModule {
#[pin]
_miscdev: MiscDeviceRegistration<RustMiscDevice>,
}
-impl kernel::InPlaceModule for RustMiscDeviceModule {
- fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
+#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
+struct RustMiscDeviceModule {
+ _faux: faux::Registration,
+ _miscdev: Pin<KBox<MiscDeviceRegistration<RustMiscDevice>>>,
+}
+
+impl RustMiscDeviceModule {
+ fn init() -> MiscDeviceOptions {
pr_info!("Initializing Rust Misc Device Sample\n");
- let options = MiscDeviceOptions {
+ MiscDeviceOptions {
name: c_str!("rust-misc-device"),
- };
+ }
+ }
+}
+
+#[cfg(not(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT))]
+impl kernel::InPlaceModule for RustMiscDeviceModule {
+ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
+ let options = Self::init();
try_pin_init!(Self {
_miscdev <- MiscDeviceRegistration::register(
@@ -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
+ // `Result<Pin<KBox<T>>>`, but faux always binds to a "dummy" driver, hence probe() is
+ // not required.
+ let misc = KBox::pin_init(
+ MiscDeviceRegistration::register(
+ options,
+ Arc::pin_init(new_mutex!(Inner { value: 0_i32 }), GFP_KERNEL),
+ Some(faux.as_ref()),
+ ),
+ GFP_KERNEL,
+ )?;
+
+ Ok(Self {
+ _faux: faux,
+ _miscdev: misc,
+ })
+ }
+}
+
struct Inner {
value: i32,
}
--
2.49.0On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
> In order to demonstrate and test a MiscDeviceRegistration with a parent
> device, introduce CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT.
>
> If CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT=y the misc device sample
> is initialized with a parent device (faux), otherwise it is initialized
> without a parent device, i.e. the exact same way as without this patch.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
> samples/rust/Kconfig | 8 +++++
> samples/rust/rust_misc_device.rs | 50 +++++++++++++++++++++++++++++---
> 2 files changed, 54 insertions(+), 4 deletions(-)
>
> diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> index b1006ab4bc3c..9948ec0939ef 100644
> --- a/samples/rust/Kconfig
> +++ b/samples/rust/Kconfig
> @@ -30,6 +30,14 @@ config SAMPLE_RUST_MISC_DEVICE
>
> If unsure, say N.
>
> +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.
> config SAMPLE_RUST_PRINT
> tristate "Printing macros"
> help
> diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> index 9bf1a0f64e6e..175638d6d341 100644
> --- a/samples/rust/rust_misc_device.rs
> +++ b/samples/rust/rust_misc_device.rs
> @@ -167,6 +167,9 @@
> uaccess::{UserSlice, UserSliceReader, UserSliceWriter},
> };
>
> +#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
> +use kernel::faux;
> +
> const RUST_MISC_DEV_HELLO: u32 = _IO('|' as u32, 0x80);
> const RUST_MISC_DEV_GET_VALUE: u32 = _IOR::<i32>('|' as u32, 0x81);
> const RUST_MISC_DEV_SET_VALUE: u32 = _IOW::<i32>('|' as u32, 0x82);
> @@ -181,19 +184,33 @@
> license: "GPL",
> }
>
> +#[cfg(not(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT))]
> #[pin_data]
> struct RustMiscDeviceModule {
> #[pin]
> _miscdev: MiscDeviceRegistration<RustMiscDevice>,
> }
>
> -impl kernel::InPlaceModule for RustMiscDeviceModule {
> - fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
> +#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
> +struct RustMiscDeviceModule {
> + _faux: faux::Registration,
> + _miscdev: Pin<KBox<MiscDeviceRegistration<RustMiscDevice>>>,
> +}
> +
> +impl RustMiscDeviceModule {
> + fn init() -> MiscDeviceOptions {
> pr_info!("Initializing Rust Misc Device Sample\n");
>
> - let options = MiscDeviceOptions {
> + MiscDeviceOptions {
> name: c_str!("rust-misc-device"),
> - };
> + }
> + }
> +}
> +
> +#[cfg(not(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT))]
> +impl kernel::InPlaceModule for RustMiscDeviceModule {
> + fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
> + let options = Self::init();
>
> try_pin_init!(Self {
> _miscdev <- MiscDeviceRegistration::register(
> @@ -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
> + // `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`?
> + // not required.
> + let misc = KBox::pin_init(
> + MiscDeviceRegistration::register(
> + options,
> + Arc::pin_init(new_mutex!(Inner { value: 0_i32 }), GFP_KERNEL),
> + Some(faux.as_ref()),
> + ),
> + GFP_KERNEL,
> + )?;
You could also initialize this module variation in-place. (this would
also require the pin-init change to reference initialized fields)
---
Cheers,
Benno
> +
> + Ok(Self {
> + _faux: faux,
> + _miscdev: misc,
> + })
> + }
> +}
> +
> struct Inner {
> value: i32,
> }
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:
> > In order to demonstrate and test a MiscDeviceRegistration with a parent
> > device, introduce CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT.
> >
> > If CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT=y the misc device sample
> > is initialized with a parent device (faux), otherwise it is initialized
> > without a parent device, i.e. the exact same way as without this patch.
> >
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > ---
> > samples/rust/Kconfig | 8 +++++
> > samples/rust/rust_misc_device.rs | 50 +++++++++++++++++++++++++++++---
> > 2 files changed, 54 insertions(+), 4 deletions(-)
> >
> > diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> > index b1006ab4bc3c..9948ec0939ef 100644
> > --- a/samples/rust/Kconfig
> > +++ b/samples/rust/Kconfig
> > @@ -30,6 +30,14 @@ config SAMPLE_RUST_MISC_DEVICE
> >
> > If unsure, say N.
> >
> > +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.
> > config SAMPLE_RUST_PRINT
> > tristate "Printing macros"
> > help
> > diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> > index 9bf1a0f64e6e..175638d6d341 100644
> > --- a/samples/rust/rust_misc_device.rs
> > +++ b/samples/rust/rust_misc_device.rs
> > @@ -167,6 +167,9 @@
> > uaccess::{UserSlice, UserSliceReader, UserSliceWriter},
> > };
> >
> > +#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
> > +use kernel::faux;
> > +
> > const RUST_MISC_DEV_HELLO: u32 = _IO('|' as u32, 0x80);
> > const RUST_MISC_DEV_GET_VALUE: u32 = _IOR::<i32>('|' as u32, 0x81);
> > const RUST_MISC_DEV_SET_VALUE: u32 = _IOW::<i32>('|' as u32, 0x82);
> > @@ -181,19 +184,33 @@
> > license: "GPL",
> > }
> >
> > +#[cfg(not(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT))]
> > #[pin_data]
> > struct RustMiscDeviceModule {
> > #[pin]
> > _miscdev: MiscDeviceRegistration<RustMiscDevice>,
> > }
> >
> > -impl kernel::InPlaceModule for RustMiscDeviceModule {
> > - fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
> > +#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
> > +struct RustMiscDeviceModule {
> > + _faux: faux::Registration,
> > + _miscdev: Pin<KBox<MiscDeviceRegistration<RustMiscDevice>>>,
> > +}
> > +
> > +impl RustMiscDeviceModule {
> > + fn init() -> MiscDeviceOptions {
> > pr_info!("Initializing Rust Misc Device Sample\n");
> >
> > - let options = MiscDeviceOptions {
> > + MiscDeviceOptions {
> > name: c_str!("rust-misc-device"),
> > - };
> > + }
> > + }
> > +}
> > +
> > +#[cfg(not(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT))]
> > +impl kernel::InPlaceModule for RustMiscDeviceModule {
> > + fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
> > + let options = Self::init();
> >
> > try_pin_init!(Self {
> > _miscdev <- MiscDeviceRegistration::register(
> > @@ -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
> > + // `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`.
> > + // not required.
> > + let misc = KBox::pin_init(
> > + MiscDeviceRegistration::register(
> > + options,
> > + Arc::pin_init(new_mutex!(Inner { value: 0_i32 }), GFP_KERNEL),
> > + Some(faux.as_ref()),
> > + ),
> > + GFP_KERNEL,
> > + )?;
>
> You could also initialize this module variation in-place. (this would
> also require the pin-init change to reference initialized fields)
Yes, I also thought about that. But this way is a bit closer to what things
would look like within a probe() callback.
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:
>> > In order to demonstrate and test a MiscDeviceRegistration with a parent
>> > device, introduce CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT.
>> >
>> > If CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT=y the misc device sample
>> > is initialized with a parent device (faux), otherwise it is initialized
>> > without a parent device, i.e. the exact same way as without this patch.
>> >
>> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>> > ---
>> > samples/rust/Kconfig | 8 +++++
>> > samples/rust/rust_misc_device.rs | 50 +++++++++++++++++++++++++++++---
>> > 2 files changed, 54 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
>> > index b1006ab4bc3c..9948ec0939ef 100644
>> > --- a/samples/rust/Kconfig
>> > +++ b/samples/rust/Kconfig
>> > @@ -30,6 +30,14 @@ config SAMPLE_RUST_MISC_DEVICE
>> >
>> > If unsure, say N.
>> >
>> > +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)
I really would like to avoid `cfg` in samples.
>> > config SAMPLE_RUST_PRINT
>> > tristate "Printing macros"
>> > help
>> > diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
>> > index 9bf1a0f64e6e..175638d6d341 100644
>> > --- a/samples/rust/rust_misc_device.rs
>> > +++ b/samples/rust/rust_misc_device.rs
>> > @@ -167,6 +167,9 @@
>> > uaccess::{UserSlice, UserSliceReader, UserSliceWriter},
>> > };
>> >
>> > +#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
>> > +use kernel::faux;
>> > +
>> > const RUST_MISC_DEV_HELLO: u32 = _IO('|' as u32, 0x80);
>> > const RUST_MISC_DEV_GET_VALUE: u32 = _IOR::<i32>('|' as u32, 0x81);
>> > const RUST_MISC_DEV_SET_VALUE: u32 = _IOW::<i32>('|' as u32, 0x82);
>> > @@ -181,19 +184,33 @@
>> > license: "GPL",
>> > }
>> >
>> > +#[cfg(not(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT))]
>> > #[pin_data]
>> > struct RustMiscDeviceModule {
>> > #[pin]
>> > _miscdev: MiscDeviceRegistration<RustMiscDevice>,
>> > }
>> >
>> > -impl kernel::InPlaceModule for RustMiscDeviceModule {
>> > - fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
>> > +#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
>> > +struct RustMiscDeviceModule {
>> > + _faux: faux::Registration,
>> > + _miscdev: Pin<KBox<MiscDeviceRegistration<RustMiscDevice>>>,
>> > +}
>> > +
>> > +impl RustMiscDeviceModule {
>> > + fn init() -> MiscDeviceOptions {
>> > pr_info!("Initializing Rust Misc Device Sample\n");
>> >
>> > - let options = MiscDeviceOptions {
>> > + MiscDeviceOptions {
>> > name: c_str!("rust-misc-device"),
>> > - };
>> > + }
>> > + }
>> > +}
>> > +
>> > +#[cfg(not(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT))]
>> > +impl kernel::InPlaceModule for RustMiscDeviceModule {
>> > + fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
>> > + let options = Self::init();
>> >
>> > try_pin_init!(Self {
>> > _miscdev <- MiscDeviceRegistration::register(
>> > @@ -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?
Another thing: faux devices don't have a `probe` in rust, so saying "not
required" doesn't make much sense, right?
>> > + // not required.
>> > + let misc = KBox::pin_init(
>> > + MiscDeviceRegistration::register(
>> > + options,
>> > + Arc::pin_init(new_mutex!(Inner { value: 0_i32 }), GFP_KERNEL),
>> > + Some(faux.as_ref()),
>> > + ),
>> > + GFP_KERNEL,
>> > + )?;
>>
>> You could also initialize this module variation in-place. (this would
>> also require the pin-init change to reference initialized fields)
>
> Yes, I also thought about that. But this way is a bit closer to what things
> would look like within a probe() callback.
Yeah then let's do that :)
---
Cheers,
Benno
On Sat, May 31, 2025 at 10:11 AM Benno Lossin <lossin@kernel.org> wrote: > > 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) It does, in the sense that you can use Rust modules (i.e. different files). Multi-file in the sense of linking several C files + a single Rust crate root also works (we have a sample doing that). Reusing the same `mod` from two different crate roots should also work (well, unless `rustc` doesn't like it for some reason, but from a quick test it seems OK), plus games with `#[path]` and symlinks. > I really would like to avoid `cfg` in samples. I think the `cfg` is not the end of the world w.r.t. learning (after all, `cfg`s are part of the kernel all the time, and it is not the first sample). In fact, one could argue that it is a good way to show what `cfg` can do, in a way. But another disadvantage of `cfg` is that then one cannot have both modules at the same time, and thus 2 builds to test both etc. Cheers, Miguel
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:
> >> > In order to demonstrate and test a MiscDeviceRegistration with a parent
> >> > device, introduce CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT.
> >> >
> >> > If CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT=y the misc device sample
> >> > is initialized with a parent device (faux), otherwise it is initialized
> >> > without a parent device, i.e. the exact same way as without this patch.
> >> >
> >> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> >> > ---
> >> > samples/rust/Kconfig | 8 +++++
> >> > samples/rust/rust_misc_device.rs | 50 +++++++++++++++++++++++++++++---
> >> > 2 files changed, 54 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> >> > index b1006ab4bc3c..9948ec0939ef 100644
> >> > --- a/samples/rust/Kconfig
> >> > +++ b/samples/rust/Kconfig
> >> > @@ -30,6 +30,14 @@ config SAMPLE_RUST_MISC_DEVICE
> >> >
> >> > If unsure, say N.
> >> >
> >> > +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.
>
> I really would like to avoid `cfg` in samples.
>
> >> > config SAMPLE_RUST_PRINT
> >> > tristate "Printing macros"
> >> > help
> >> > diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> >> > index 9bf1a0f64e6e..175638d6d341 100644
> >> > --- a/samples/rust/rust_misc_device.rs
> >> > +++ b/samples/rust/rust_misc_device.rs
> >> > @@ -167,6 +167,9 @@
> >> > uaccess::{UserSlice, UserSliceReader, UserSliceWriter},
> >> > };
> >> >
> >> > +#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
> >> > +use kernel::faux;
> >> > +
> >> > const RUST_MISC_DEV_HELLO: u32 = _IO('|' as u32, 0x80);
> >> > const RUST_MISC_DEV_GET_VALUE: u32 = _IOR::<i32>('|' as u32, 0x81);
> >> > const RUST_MISC_DEV_SET_VALUE: u32 = _IOW::<i32>('|' as u32, 0x82);
> >> > @@ -181,19 +184,33 @@
> >> > license: "GPL",
> >> > }
> >> >
> >> > +#[cfg(not(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT))]
> >> > #[pin_data]
> >> > struct RustMiscDeviceModule {
> >> > #[pin]
> >> > _miscdev: MiscDeviceRegistration<RustMiscDevice>,
> >> > }
> >> >
> >> > -impl kernel::InPlaceModule for RustMiscDeviceModule {
> >> > - fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
> >> > +#[cfg(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT)]
> >> > +struct RustMiscDeviceModule {
> >> > + _faux: faux::Registration,
> >> > + _miscdev: Pin<KBox<MiscDeviceRegistration<RustMiscDevice>>>,
> >> > +}
> >> > +
> >> > +impl RustMiscDeviceModule {
> >> > + fn init() -> MiscDeviceOptions {
> >> > pr_info!("Initializing Rust Misc Device Sample\n");
> >> >
> >> > - let options = MiscDeviceOptions {
> >> > + MiscDeviceOptions {
> >> > name: c_str!("rust-misc-device"),
> >> > - };
> >> > + }
> >> > + }
> >> > +}
> >> > +
> >> > +#[cfg(not(CONFIG_SAMPLE_RUST_MISC_DEVICE_WITH_PARENT))]
> >> > +impl kernel::InPlaceModule for RustMiscDeviceModule {
> >> > + fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
> >> > + let options = Self::init();
> >> >
> >> > try_pin_init!(Self {
> >> > _miscdev <- MiscDeviceRegistration::register(
> >> > @@ -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.
> >> > + // not required.
> >> > + let misc = KBox::pin_init(
> >> > + MiscDeviceRegistration::register(
> >> > + options,
> >> > + Arc::pin_init(new_mutex!(Inner { value: 0_i32 }), GFP_KERNEL),
> >> > + Some(faux.as_ref()),
> >> > + ),
> >> > + GFP_KERNEL,
> >> > + )?;
> >>
> >> You could also initialize this module variation in-place. (this would
> >> also require the pin-init change to reference initialized fields)
> >
> > Yes, I also thought about that. But this way is a bit closer to what things
> > would look like within a probe() callback.
>
> Yeah then let's do that :)
>
> ---
> Cheers,
> Benno
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
On Sat, May 31, 2025 at 02:03:05PM +0200, Benno Lossin wrote:
> 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 :)
Seems reasonable to me -- let's do that then.
© 2016 - 2025 Red Hat, Inc.