Re: [PATCH 7/7] rust: sample: misc: implement device driver sample

["Benno Lossin" <lossin@kernel.org>]

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