[PATCH 1/2] samples: rust: Provide example using the new Rust MiscDevice abstraction

[PATCH 1/2] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Lee Jones]

This sample driver demonstrates the following basic operations:

* Register a Misc Device
* Create /dev/rust-misc-device
* Open the aforementioned character device
* Operate on the character device via a simple ioctl()
* Close the character device

Signed-off-by: Lee Jones <lee@kernel.org>
---
 samples/rust/Kconfig             | 10 ++++
 samples/rust/Makefile            |  1 +
 samples/rust/rust_misc_device.rs | 84 ++++++++++++++++++++++++++++++++
 3 files changed, 95 insertions(+)
 create mode 100644 samples/rust/rust_misc_device.rs

diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
index b0f74a81c8f9..df384e679901 100644
--- a/samples/rust/Kconfig
+++ b/samples/rust/Kconfig
@@ -20,6 +20,16 @@ config SAMPLE_RUST_MINIMAL
 
 	  If unsure, say N.
 
+config SAMPLE_RUST_MISC_DEVICE
+	tristate "Misc device"
+	help
+	  This option builds the Rust misc device.
+
+	  To compile this as a module, choose M here:
+	  the module will be called rust_misc_device.
+
+	  If unsure, say N.
+
 config SAMPLE_RUST_PRINT
 	tristate "Printing macros"
 	help
diff --git a/samples/rust/Makefile b/samples/rust/Makefile
index c1a5c1655395..ad4b97a98580 100644
--- a/samples/rust/Makefile
+++ b/samples/rust/Makefile
@@ -2,6 +2,7 @@
 ccflags-y += -I$(src)				# needed for trace events
 
 obj-$(CONFIG_SAMPLE_RUST_MINIMAL)		+= rust_minimal.o
+obj-$(CONFIG_SAMPLE_RUST_MISC_DEVICE)		+= rust_misc_device.o
 obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
 
 rust_print-y := rust_print_main.o rust_print_events.o
diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
new file mode 100644
index 000000000000..5f1b69569ef7
--- /dev/null
+++ b/samples/rust/rust_misc_device.rs
@@ -0,0 +1,84 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Rust misc device sample.
+
+use kernel::{
+    c_str,
+    ioctl::_IO,
+    miscdevice::{MiscDevice, MiscDeviceOptions, MiscDeviceRegistration},
+    prelude::*,
+};
+
+const RUST_MISC_DEV_HELLO: u32 = _IO('R' as u32, 9);
+
+module! {
+    type: RustMiscDeviceModule,
+    name: "rust_misc_device",
+    author: "Lee Jones",
+    description: "Rust misc device sample",
+    license: "GPL",
+}
+
+struct RustMiscDeviceModule {
+    _miscdev: Pin<KBox<MiscDeviceRegistration<RustMiscDevice>>>,
+}
+
+impl kernel::Module for RustMiscDeviceModule {
+    fn init(_module: &'static ThisModule) -> Result<Self> {
+        pr_info!("Initialising Rust Misc Device Sample\n");
+
+        let options = MiscDeviceOptions {
+            name: c_str!("rust-misc-device"),
+        };
+
+        Ok(Self {
+            _miscdev: KBox::pin_init(
+                MiscDeviceRegistration::<RustMiscDevice>::register(options),
+                GFP_KERNEL,
+            )?,
+        })
+    }
+}
+
+struct RustMiscDevice;
+
+impl RustMiscDevice {
+    fn new() -> Self {
+        Self
+    }
+}
+
+#[vtable]
+impl MiscDevice for RustMiscDevice {
+    type Ptr = KBox<Self>;
+
+    fn open() -> Result<KBox<Self>> {
+        pr_info!("Opening Rust Misc Device Sample\n");
+
+        Ok(KBox::new(RustMiscDevice::new(), GFP_KERNEL)?)
+    }
+
+    fn ioctl(
+        _device: <Self::Ptr as kernel::types::ForeignOwnable>::Borrowed<'_>,
+        cmd: u32,
+        _arg: usize,
+    ) -> Result<isize> {
+        pr_info!("IOCTLing Rust Misc Device Sample\n");
+
+        match cmd {
+            RUST_MISC_DEV_HELLO => pr_info!("Hello from the Rust Misc Device\n"),
+            _ => {
+                pr_err!("IOCTL not recognised: {}\n", cmd);
+                return Err(EINVAL);
+            }
+        }
+
+        Ok(0)
+    }
+}
+
+impl Drop for RustMiscDevice {
+    fn drop(&mut self) {
+        pr_info!("Exiting the Rust Misc Device Sample\n");
+    }
+}
-- 
2.47.0.338.g60cca15819-goog

[PATCH 2/2] sample: rust_misc_device: Demonstrate additional get/set value functionality

[Lee Jones]

Expand the complexity of the sample driver by providing the ability to
get and set an integer.  The value is protected by a mutex.

Here is a simple userspace program that fully exercises the sample
driver's capabilities.

int main() {
  int value, new_value;
  int fd, ret;

  // Open the device file
  printf("Opening /dev/rust-misc-device for reading and writing\n");
  fd = open("/dev/rust-misc-device", O_RDWR);
  if (fd < 0) {
    perror("open");
    return errno;
  }

  // Make call into driver to say "hello"
  printf("Calling Hello\n");
  ret = ioctl(fd, RUST_MISC_DEV_HELLO, NULL);
  if (ret < 0) {
    perror("ioctl: Failed to call into Hello");
    close(fd);
    return errno;
  }

  // Get initial value
  printf("Fetching initial value\n");
  ret = ioctl(fd, RUST_MISC_DEV_GET_VALUE, &value);
  if (ret < 0) {
    perror("ioctl: Failed to fetch the initial value");
    close(fd);
    return errno;
  }

  value++;

  // Set value to something different
  printf("Submitting new value (%d)\n", value);
  ret = ioctl(fd, RUST_MISC_DEV_SET_VALUE, &value);
  if (ret < 0) {
    perror("ioctl: Failed to submit new value");
    close(fd);
    return errno;
  }

  // Ensure new value was applied
  printf("Fetching new value\n");
  ret = ioctl(fd, RUST_MISC_DEV_GET_VALUE, &new_value);
  if (ret < 0) {
    perror("ioctl: Failed to fetch the new value");
    close(fd);
    return errno;
  }

  if (value != new_value) {
    printf("Failed: Committed and retrieved values are different (%d - %d)\n", value, new_value);
    close(fd);
    return -1;
  }

  // Call the unsuccessful ioctl
  printf("Attempting to call in to an non-existent IOCTL\n");
  ret = ioctl(fd, RUST_MISC_DEV_FAIL, NULL);
  if (ret < 0) {
    perror("ioctl: Succeeded to fail - this was expected");
  } else {
    printf("ioctl: Failed to fail\n");
    close(fd);
    return -1;
  }

  // Close the device file
  printf("Closing /dev/rust-misc-device\n");
  close(fd);

  printf("Success\n");
  return 0;
}

Signed-off-by: Lee Jones <lee@kernel.org>
---
 samples/rust/rust_misc_device.rs | 82 ++++++++++++++++++++++++--------
 1 file changed, 62 insertions(+), 20 deletions(-)

diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
index 5f1b69569ef7..9c041497d881 100644
--- a/samples/rust/rust_misc_device.rs
+++ b/samples/rust/rust_misc_device.rs
@@ -2,13 +2,20 @@
 
 //! Rust misc device sample.
 
+use core::pin::Pin;
+
 use kernel::{
     c_str,
-    ioctl::_IO,
+    ioctl::{_IO, _IOC_SIZE, _IOR, _IOW},
     miscdevice::{MiscDevice, MiscDeviceOptions, MiscDeviceRegistration},
+    new_mutex,
     prelude::*,
+    sync::Mutex,
+    uaccess::{UserSlice, UserSliceReader, UserSliceWriter},
 };
 
+const RUST_MISC_DEV_GET_VALUE: u32 = _IOR::<i32>('R' as u32, 7);
+const RUST_MISC_DEV_SET_VALUE: u32 = _IOW::<i32>('R' as u32, 8);
 const RUST_MISC_DEV_HELLO: u32 = _IO('R' as u32, 9);
 
 module! {
@@ -40,45 +47,80 @@ fn init(_module: &'static ThisModule) -> Result<Self> {
     }
 }
 
-struct RustMiscDevice;
+struct Inner {
+    value: i32,
+}
 
-impl RustMiscDevice {
-    fn new() -> Self {
-        Self
-    }
+#[pin_data(PinnedDrop)]
+struct RustMiscDevice {
+    #[pin]
+    inner: Mutex<Inner>,
 }
 
 #[vtable]
 impl MiscDevice for RustMiscDevice {
-    type Ptr = KBox<Self>;
+    type Ptr = Pin<KBox<Self>>;
 
-    fn open() -> Result<KBox<Self>> {
+    fn open() -> Result<Pin<KBox<Self>>> {
         pr_info!("Opening Rust Misc Device Sample\n");
 
-        Ok(KBox::new(RustMiscDevice::new(), GFP_KERNEL)?)
+        KBox::try_pin_init(
+            try_pin_init! {
+                RustMiscDevice { inner <- new_mutex!( Inner{ value: 0_i32 } )}
+            },
+            GFP_KERNEL,
+        )
     }
 
-    fn ioctl(
-        _device: <Self::Ptr as kernel::types::ForeignOwnable>::Borrowed<'_>,
-        cmd: u32,
-        _arg: usize,
-    ) -> Result<isize> {
+    fn ioctl(device: Pin<&RustMiscDevice>, cmd: u32, arg: usize) -> Result<isize> {
         pr_info!("IOCTLing Rust Misc Device Sample\n");
 
-        match cmd {
-            RUST_MISC_DEV_HELLO => pr_info!("Hello from the Rust Misc Device\n"),
+        let size = _IOC_SIZE(cmd);
+
+        let _ = match cmd {
+            RUST_MISC_DEV_GET_VALUE => device.get_value(UserSlice::new(arg, size).writer())?,
+            RUST_MISC_DEV_SET_VALUE => device.set_value(UserSlice::new(arg, size).reader())?,
+            RUST_MISC_DEV_HELLO => device.hello()?,
             _ => {
-                pr_err!("IOCTL not recognised: {}\n", cmd);
+                pr_err!("-> IOCTL not recognised: {}\n", cmd);
                 return Err(EINVAL);
             }
-        }
+        };
 
         Ok(0)
     }
 }
 
-impl Drop for RustMiscDevice {
-    fn drop(&mut self) {
+#[pinned_drop]
+impl PinnedDrop for RustMiscDevice {
+    fn drop(self: Pin<&mut Self>) {
         pr_info!("Exiting the Rust Misc Device Sample\n");
     }
 }
+
+impl RustMiscDevice {
+    fn set_value(&self, mut reader: UserSliceReader) -> Result<isize> {
+        let new_value = reader.read::<i32>()?;
+        let mut guard = self.inner.lock();
+
+        pr_info!("-> Copying data from userspace (value: {})\n", new_value);
+
+        guard.value = new_value;
+        Ok(0)
+    }
+
+    fn get_value(&self, mut writer: UserSliceWriter) -> Result<isize> {
+        let guard = self.inner.lock();
+
+        pr_info!("-> Copying data to userspace (value: {})\n", &guard.value);
+
+        writer.write::<i32>(&guard.value)?;
+        Ok(0)
+    }
+
+    fn hello(&self) -> Result<isize> {
+        pr_info!("-> Hello from the Rust Misc Device\n");
+
+        Ok(0)
+    }
+}
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH 2/2] sample: rust_misc_device: Demonstrate additional get/set value functionality

[Greg KH]

On Wed, Dec 04, 2024 at 05:46:25PM +0000, Lee Jones wrote:
> Expand the complexity of the sample driver by providing the ability to
> get and set an integer.  The value is protected by a mutex.
> 
> Here is a simple userspace program that fully exercises the sample
> driver's capabilities.
> 
> int main() {
>   int value, new_value;
>   int fd, ret;
> 
>   // Open the device file
>   printf("Opening /dev/rust-misc-device for reading and writing\n");
>   fd = open("/dev/rust-misc-device", O_RDWR);
>   if (fd < 0) {
>     perror("open");
>     return errno;
>   }
> 
>   // Make call into driver to say "hello"
>   printf("Calling Hello\n");
>   ret = ioctl(fd, RUST_MISC_DEV_HELLO, NULL);
>   if (ret < 0) {
>     perror("ioctl: Failed to call into Hello");
>     close(fd);
>     return errno;
>   }
> 
>   // Get initial value
>   printf("Fetching initial value\n");
>   ret = ioctl(fd, RUST_MISC_DEV_GET_VALUE, &value);
>   if (ret < 0) {
>     perror("ioctl: Failed to fetch the initial value");
>     close(fd);
>     return errno;
>   }
> 
>   value++;
> 
>   // Set value to something different
>   printf("Submitting new value (%d)\n", value);
>   ret = ioctl(fd, RUST_MISC_DEV_SET_VALUE, &value);
>   if (ret < 0) {
>     perror("ioctl: Failed to submit new value");
>     close(fd);
>     return errno;
>   }
> 
>   // Ensure new value was applied
>   printf("Fetching new value\n");
>   ret = ioctl(fd, RUST_MISC_DEV_GET_VALUE, &new_value);
>   if (ret < 0) {
>     perror("ioctl: Failed to fetch the new value");
>     close(fd);
>     return errno;
>   }
> 
>   if (value != new_value) {
>     printf("Failed: Committed and retrieved values are different (%d - %d)\n", value, new_value);
>     close(fd);
>     return -1;
>   }
> 
>   // Call the unsuccessful ioctl
>   printf("Attempting to call in to an non-existent IOCTL\n");
>   ret = ioctl(fd, RUST_MISC_DEV_FAIL, NULL);
>   if (ret < 0) {
>     perror("ioctl: Succeeded to fail - this was expected");
>   } else {
>     printf("ioctl: Failed to fail\n");
>     close(fd);
>     return -1;
>   }
> 
>   // Close the device file
>   printf("Closing /dev/rust-misc-device\n");
>   close(fd);
> 
>   printf("Success\n");
>   return 0;
> }
> 
> Signed-off-by: Lee Jones <lee@kernel.org>
> ---
>  samples/rust/rust_misc_device.rs | 82 ++++++++++++++++++++++++--------
>  1 file changed, 62 insertions(+), 20 deletions(-)
> 
> diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> index 5f1b69569ef7..9c041497d881 100644
> --- a/samples/rust/rust_misc_device.rs
> +++ b/samples/rust/rust_misc_device.rs
> @@ -2,13 +2,20 @@
>  
>  //! Rust misc device sample.
>  
> +use core::pin::Pin;
> +
>  use kernel::{
>      c_str,
> -    ioctl::_IO,
> +    ioctl::{_IO, _IOC_SIZE, _IOR, _IOW},
>      miscdevice::{MiscDevice, MiscDeviceOptions, MiscDeviceRegistration},
> +    new_mutex,
>      prelude::*,
> +    sync::Mutex,
> +    uaccess::{UserSlice, UserSliceReader, UserSliceWriter},
>  };
>  
> +const RUST_MISC_DEV_GET_VALUE: u32 = _IOR::<i32>('R' as u32, 7);
> +const RUST_MISC_DEV_SET_VALUE: u32 = _IOW::<i32>('R' as u32, 8);

Shouldn't this be 'W'?

>  const RUST_MISC_DEV_HELLO: u32 = _IO('R' as u32, 9);
>  
>  module! {
> @@ -40,45 +47,80 @@ fn init(_module: &'static ThisModule) -> Result<Self> {
>      }
>  }
>  
> -struct RustMiscDevice;
> +struct Inner {
> +    value: i32,
> +}
>  
> -impl RustMiscDevice {
> -    fn new() -> Self {
> -        Self
> -    }
> +#[pin_data(PinnedDrop)]
> +struct RustMiscDevice {
> +    #[pin]
> +    inner: Mutex<Inner>,
>  }
>  
>  #[vtable]
>  impl MiscDevice for RustMiscDevice {
> -    type Ptr = KBox<Self>;
> +    type Ptr = Pin<KBox<Self>>;
>  
> -    fn open() -> Result<KBox<Self>> {
> +    fn open() -> Result<Pin<KBox<Self>>> {
>          pr_info!("Opening Rust Misc Device Sample\n");
>  
> -        Ok(KBox::new(RustMiscDevice::new(), GFP_KERNEL)?)
> +        KBox::try_pin_init(
> +            try_pin_init! {
> +                RustMiscDevice { inner <- new_mutex!( Inner{ value: 0_i32 } )}
> +            },
> +            GFP_KERNEL,
> +        )
>      }
>  
> -    fn ioctl(
> -        _device: <Self::Ptr as kernel::types::ForeignOwnable>::Borrowed<'_>,
> -        cmd: u32,
> -        _arg: usize,
> -    ) -> Result<isize> {
> +    fn ioctl(device: Pin<&RustMiscDevice>, cmd: u32, arg: usize) -> Result<isize> {
>          pr_info!("IOCTLing Rust Misc Device Sample\n");
>  
> -        match cmd {
> -            RUST_MISC_DEV_HELLO => pr_info!("Hello from the Rust Misc Device\n"),
> +        let size = _IOC_SIZE(cmd);
> +
> +        let _ = match cmd {
> +            RUST_MISC_DEV_GET_VALUE => device.get_value(UserSlice::new(arg, size).writer())?,
> +            RUST_MISC_DEV_SET_VALUE => device.set_value(UserSlice::new(arg, size).reader())?,
> +            RUST_MISC_DEV_HELLO => device.hello()?,
>              _ => {
> -                pr_err!("IOCTL not recognised: {}\n", cmd);
> +                pr_err!("-> IOCTL not recognised: {}\n", cmd);
>                  return Err(EINVAL);

Nit, wrong return value for an invalid ioctl, I missed that in patch 1,
sorry about that.

>              }
> -        }
> +        };
>  
>          Ok(0)
>      }
>  }
>  
> -impl Drop for RustMiscDevice {
> -    fn drop(&mut self) {
> +#[pinned_drop]
> +impl PinnedDrop for RustMiscDevice {
> +    fn drop(self: Pin<&mut Self>) {
>          pr_info!("Exiting the Rust Misc Device Sample\n");
>      }
>  }
> +
> +impl RustMiscDevice {
> +    fn set_value(&self, mut reader: UserSliceReader) -> Result<isize> {
> +        let new_value = reader.read::<i32>()?;
> +        let mut guard = self.inner.lock();
> +
> +        pr_info!("-> Copying data from userspace (value: {})\n", new_value);
> +
> +        guard.value = new_value;
> +        Ok(0)
> +    }
> +
> +    fn get_value(&self, mut writer: UserSliceWriter) -> Result<isize> {
> +        let guard = self.inner.lock();
> +
> +        pr_info!("-> Copying data to userspace (value: {})\n", &guard.value);
> +
> +        writer.write::<i32>(&guard.value)?;

What happens if it fails, shouldn't your pr_info() happen after this?

thanks,

greg k-h

> +        Ok(0)
> +    }
> +
> +    fn hello(&self) -> Result<isize> {
> +        pr_info!("-> Hello from the Rust Misc Device\n");
> +
> +        Ok(0)
> +    }
> +}
> -- 
> 2.47.0.338.g60cca15819-goog
> 

Re: [PATCH 2/2] sample: rust_misc_device: Demonstrate additional get/set value functionality

[Lee Jones]

On Wed, 04 Dec 2024, Greg KH wrote:

> On Wed, Dec 04, 2024 at 05:46:25PM +0000, Lee Jones wrote:
> > Expand the complexity of the sample driver by providing the ability to
> > get and set an integer.  The value is protected by a mutex.
> > 
> > Here is a simple userspace program that fully exercises the sample
> > driver's capabilities.
> > 
> > int main() {
> >   int value, new_value;
> >   int fd, ret;
> > 
> >   // Open the device file
> >   printf("Opening /dev/rust-misc-device for reading and writing\n");
> >   fd = open("/dev/rust-misc-device", O_RDWR);
> >   if (fd < 0) {
> >     perror("open");
> >     return errno;
> >   }
> > 
> >   // Make call into driver to say "hello"
> >   printf("Calling Hello\n");
> >   ret = ioctl(fd, RUST_MISC_DEV_HELLO, NULL);
> >   if (ret < 0) {
> >     perror("ioctl: Failed to call into Hello");
> >     close(fd);
> >     return errno;
> >   }
> > 
> >   // Get initial value
> >   printf("Fetching initial value\n");
> >   ret = ioctl(fd, RUST_MISC_DEV_GET_VALUE, &value);
> >   if (ret < 0) {
> >     perror("ioctl: Failed to fetch the initial value");
> >     close(fd);
> >     return errno;
> >   }
> > 
> >   value++;
> > 
> >   // Set value to something different
> >   printf("Submitting new value (%d)\n", value);
> >   ret = ioctl(fd, RUST_MISC_DEV_SET_VALUE, &value);
> >   if (ret < 0) {
> >     perror("ioctl: Failed to submit new value");
> >     close(fd);
> >     return errno;
> >   }
> > 
> >   // Ensure new value was applied
> >   printf("Fetching new value\n");
> >   ret = ioctl(fd, RUST_MISC_DEV_GET_VALUE, &new_value);
> >   if (ret < 0) {
> >     perror("ioctl: Failed to fetch the new value");
> >     close(fd);
> >     return errno;
> >   }
> > 
> >   if (value != new_value) {
> >     printf("Failed: Committed and retrieved values are different (%d - %d)\n", value, new_value);
> >     close(fd);
> >     return -1;
> >   }
> > 
> >   // Call the unsuccessful ioctl
> >   printf("Attempting to call in to an non-existent IOCTL\n");
> >   ret = ioctl(fd, RUST_MISC_DEV_FAIL, NULL);
> >   if (ret < 0) {
> >     perror("ioctl: Succeeded to fail - this was expected");
> >   } else {
> >     printf("ioctl: Failed to fail\n");
> >     close(fd);
> >     return -1;
> >   }
> > 
> >   // Close the device file
> >   printf("Closing /dev/rust-misc-device\n");
> >   close(fd);
> > 
> >   printf("Success\n");
> >   return 0;
> > }
> > 
> > Signed-off-by: Lee Jones <lee@kernel.org>
> > ---
> >  samples/rust/rust_misc_device.rs | 82 ++++++++++++++++++++++++--------
> >  1 file changed, 62 insertions(+), 20 deletions(-)
> > 
> > diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> > index 5f1b69569ef7..9c041497d881 100644
> > --- a/samples/rust/rust_misc_device.rs
> > +++ b/samples/rust/rust_misc_device.rs
> > @@ -2,13 +2,20 @@
> >  
> >  //! Rust misc device sample.
> >  
> > +use core::pin::Pin;
> > +
> >  use kernel::{
> >      c_str,
> > -    ioctl::_IO,
> > +    ioctl::{_IO, _IOC_SIZE, _IOR, _IOW},
> >      miscdevice::{MiscDevice, MiscDeviceOptions, MiscDeviceRegistration},
> > +    new_mutex,
> >      prelude::*,
> > +    sync::Mutex,
> > +    uaccess::{UserSlice, UserSliceReader, UserSliceWriter},
> >  };
> >  
> > +const RUST_MISC_DEV_GET_VALUE: u32 = _IOR::<i32>('R' as u32, 7);
> > +const RUST_MISC_DEV_SET_VALUE: u32 = _IOW::<i32>('R' as u32, 8);
> 
> Shouldn't this be 'W'?

No, I don't think so.

'W' doesn't mean 'write'.  It's supposed to be a unique identifier:

'W'   00-1F  linux/watchdog.h                                        conflict!
'W'   00-1F  linux/wanrouter.h                                       conflict! (pre 3.9)
'W'   00-3F  sound/asound.h                                          conflict!
'W'   40-5F  drivers/pci/switch/switchtec.c
'W'   60-61  linux/watch_queue.h

'R' isn't registered for this either:

'R'   00-1F  linux/random.h                                          conflict!
'R'   01     linux/rfkill.h                                          conflict!
'R'   20-2F  linux/trace_mmap.h
'R'   C0-DF  net/bluetooth/rfcomm.h
'R'   E0     uapi/linux/fsl_mc.h

... but since this is just example code with no real purpose, I'm going
to hold short of registering a unique identifier for it.

> >  const RUST_MISC_DEV_HELLO: u32 = _IO('R' as u32, 9);
> >  
> >  module! {
> > @@ -40,45 +47,80 @@ fn init(_module: &'static ThisModule) -> Result<Self> {
> >      }
> >  }
> >  
> > -struct RustMiscDevice;
> > +struct Inner {
> > +    value: i32,
> > +}
> >  
> > -impl RustMiscDevice {
> > -    fn new() -> Self {
> > -        Self
> > -    }
> > +#[pin_data(PinnedDrop)]
> > +struct RustMiscDevice {
> > +    #[pin]
> > +    inner: Mutex<Inner>,
> >  }
> >  
> >  #[vtable]
> >  impl MiscDevice for RustMiscDevice {
> > -    type Ptr = KBox<Self>;
> > +    type Ptr = Pin<KBox<Self>>;
> >  
> > -    fn open() -> Result<KBox<Self>> {
> > +    fn open() -> Result<Pin<KBox<Self>>> {
> >          pr_info!("Opening Rust Misc Device Sample\n");
> >  
> > -        Ok(KBox::new(RustMiscDevice::new(), GFP_KERNEL)?)
> > +        KBox::try_pin_init(
> > +            try_pin_init! {
> > +                RustMiscDevice { inner <- new_mutex!( Inner{ value: 0_i32 } )}
> > +            },
> > +            GFP_KERNEL,
> > +        )
> >      }
> >  
> > -    fn ioctl(
> > -        _device: <Self::Ptr as kernel::types::ForeignOwnable>::Borrowed<'_>,
> > -        cmd: u32,
> > -        _arg: usize,
> > -    ) -> Result<isize> {
> > +    fn ioctl(device: Pin<&RustMiscDevice>, cmd: u32, arg: usize) -> Result<isize> {
> >          pr_info!("IOCTLing Rust Misc Device Sample\n");
> >  
> > -        match cmd {
> > -            RUST_MISC_DEV_HELLO => pr_info!("Hello from the Rust Misc Device\n"),
> > +        let size = _IOC_SIZE(cmd);
> > +
> > +        let _ = match cmd {
> > +            RUST_MISC_DEV_GET_VALUE => device.get_value(UserSlice::new(arg, size).writer())?,
> > +            RUST_MISC_DEV_SET_VALUE => device.set_value(UserSlice::new(arg, size).reader())?,
> > +            RUST_MISC_DEV_HELLO => device.hello()?,
> >              _ => {
> > -                pr_err!("IOCTL not recognised: {}\n", cmd);
> > +                pr_err!("-> IOCTL not recognised: {}\n", cmd);
> >                  return Err(EINVAL);
> 
> Nit, wrong return value for an invalid ioctl, I missed that in patch 1,
> sorry about that.

Good shout, thanks.

> >              }
> > -        }
> > +        };
> >  
> >          Ok(0)
> >      }
> >  }
> >  
> > -impl Drop for RustMiscDevice {
> > -    fn drop(&mut self) {
> > +#[pinned_drop]
> > +impl PinnedDrop for RustMiscDevice {
> > +    fn drop(self: Pin<&mut Self>) {
> >          pr_info!("Exiting the Rust Misc Device Sample\n");
> >      }
> >  }
> > +
> > +impl RustMiscDevice {
> > +    fn set_value(&self, mut reader: UserSliceReader) -> Result<isize> {
> > +        let new_value = reader.read::<i32>()?;
> > +        let mut guard = self.inner.lock();
> > +
> > +        pr_info!("-> Copying data from userspace (value: {})\n", new_value);
> > +
> > +        guard.value = new_value;
> > +        Ok(0)
> > +    }
> > +
> > +    fn get_value(&self, mut writer: UserSliceWriter) -> Result<isize> {
> > +        let guard = self.inner.lock();
> > +
> > +        pr_info!("-> Copying data to userspace (value: {})\n", &guard.value);
> > +
> > +        writer.write::<i32>(&guard.value)?;
> 
> What happens if it fails, shouldn't your pr_info() happen after this?

If this fails, I need the line in the log to show where it failed.

It says "copying" as in "attempting to copy", rather than "copied".

-- 
Lee Jones [李琼斯]

Re: [PATCH 2/2] sample: rust_misc_device: Demonstrate additional get/set value functionality

[Greg KH]

On Thu, Dec 05, 2024 at 08:38:48AM +0000, Lee Jones wrote:
> On Wed, 04 Dec 2024, Greg KH wrote:
> 
> > On Wed, Dec 04, 2024 at 05:46:25PM +0000, Lee Jones wrote:
> > > Expand the complexity of the sample driver by providing the ability to
> > > get and set an integer.  The value is protected by a mutex.
> > > 
> > > Here is a simple userspace program that fully exercises the sample
> > > driver's capabilities.
> > > 
> > > int main() {
> > >   int value, new_value;
> > >   int fd, ret;
> > > 
> > >   // Open the device file
> > >   printf("Opening /dev/rust-misc-device for reading and writing\n");
> > >   fd = open("/dev/rust-misc-device", O_RDWR);
> > >   if (fd < 0) {
> > >     perror("open");
> > >     return errno;
> > >   }
> > > 
> > >   // Make call into driver to say "hello"
> > >   printf("Calling Hello\n");
> > >   ret = ioctl(fd, RUST_MISC_DEV_HELLO, NULL);
> > >   if (ret < 0) {
> > >     perror("ioctl: Failed to call into Hello");
> > >     close(fd);
> > >     return errno;
> > >   }
> > > 
> > >   // Get initial value
> > >   printf("Fetching initial value\n");
> > >   ret = ioctl(fd, RUST_MISC_DEV_GET_VALUE, &value);
> > >   if (ret < 0) {
> > >     perror("ioctl: Failed to fetch the initial value");
> > >     close(fd);
> > >     return errno;
> > >   }
> > > 
> > >   value++;
> > > 
> > >   // Set value to something different
> > >   printf("Submitting new value (%d)\n", value);
> > >   ret = ioctl(fd, RUST_MISC_DEV_SET_VALUE, &value);
> > >   if (ret < 0) {
> > >     perror("ioctl: Failed to submit new value");
> > >     close(fd);
> > >     return errno;
> > >   }
> > > 
> > >   // Ensure new value was applied
> > >   printf("Fetching new value\n");
> > >   ret = ioctl(fd, RUST_MISC_DEV_GET_VALUE, &new_value);
> > >   if (ret < 0) {
> > >     perror("ioctl: Failed to fetch the new value");
> > >     close(fd);
> > >     return errno;
> > >   }
> > > 
> > >   if (value != new_value) {
> > >     printf("Failed: Committed and retrieved values are different (%d - %d)\n", value, new_value);
> > >     close(fd);
> > >     return -1;
> > >   }
> > > 
> > >   // Call the unsuccessful ioctl
> > >   printf("Attempting to call in to an non-existent IOCTL\n");
> > >   ret = ioctl(fd, RUST_MISC_DEV_FAIL, NULL);
> > >   if (ret < 0) {
> > >     perror("ioctl: Succeeded to fail - this was expected");
> > >   } else {
> > >     printf("ioctl: Failed to fail\n");
> > >     close(fd);
> > >     return -1;
> > >   }
> > > 
> > >   // Close the device file
> > >   printf("Closing /dev/rust-misc-device\n");
> > >   close(fd);
> > > 
> > >   printf("Success\n");
> > >   return 0;
> > > }
> > > 
> > > Signed-off-by: Lee Jones <lee@kernel.org>
> > > ---
> > >  samples/rust/rust_misc_device.rs | 82 ++++++++++++++++++++++++--------
> > >  1 file changed, 62 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> > > index 5f1b69569ef7..9c041497d881 100644
> > > --- a/samples/rust/rust_misc_device.rs
> > > +++ b/samples/rust/rust_misc_device.rs
> > > @@ -2,13 +2,20 @@
> > >  
> > >  //! Rust misc device sample.
> > >  
> > > +use core::pin::Pin;
> > > +
> > >  use kernel::{
> > >      c_str,
> > > -    ioctl::_IO,
> > > +    ioctl::{_IO, _IOC_SIZE, _IOR, _IOW},
> > >      miscdevice::{MiscDevice, MiscDeviceOptions, MiscDeviceRegistration},
> > > +    new_mutex,
> > >      prelude::*,
> > > +    sync::Mutex,
> > > +    uaccess::{UserSlice, UserSliceReader, UserSliceWriter},
> > >  };
> > >  
> > > +const RUST_MISC_DEV_GET_VALUE: u32 = _IOR::<i32>('R' as u32, 7);
> > > +const RUST_MISC_DEV_SET_VALUE: u32 = _IOW::<i32>('R' as u32, 8);
> > 
> > Shouldn't this be 'W'?
> 
> No, I don't think so.
> 
> 'W' doesn't mean 'write'.  It's supposed to be a unique identifier:
> 
> 'W'   00-1F  linux/watchdog.h                                        conflict!
> 'W'   00-1F  linux/wanrouter.h                                       conflict! (pre 3.9)
> 'W'   00-3F  sound/asound.h                                          conflict!
> 'W'   40-5F  drivers/pci/switch/switchtec.c
> 'W'   60-61  linux/watch_queue.h
> 
> 'R' isn't registered for this either:
> 
> 'R'   00-1F  linux/random.h                                          conflict!
> 'R'   01     linux/rfkill.h                                          conflict!
> 'R'   20-2F  linux/trace_mmap.h
> 'R'   C0-DF  net/bluetooth/rfcomm.h
> 'R'   E0     uapi/linux/fsl_mc.h
> 
> ... but since this is just example code with no real purpose, I'm going
> to hold short of registering a unique identifier for it.

Ah, sorry, I missed that this is the ioctl "name".  As the ptrace people
will complain, why not use a new one?  Ick, ioctl-number.rst is way out
of date, but I guess we should carve out one for "sample drivers, do not
use in anything real" use cases like here.

> > > +    fn get_value(&self, mut writer: UserSliceWriter) -> Result<isize> {
> > > +        let guard = self.inner.lock();
> > > +
> > > +        pr_info!("-> Copying data to userspace (value: {})\n", &guard.value);
> > > +
> > > +        writer.write::<i32>(&guard.value)?;
> > 
> > What happens if it fails, shouldn't your pr_info() happen after this?
> 
> If this fails, I need the line in the log to show where it failed.

pr_info() doesn't show file lines from what I remember has that changed?

But wait, this is a misc device, you should be using dev_info() and
friends here, no pr_*() stuff please.

> It says "copying" as in "attempting to copy", rather than "copied".

Fair enough, but if the copy fails, nothing gets printed out, right?

thanks,

greg k-h

Re: [PATCH 2/2] sample: rust_misc_device: Demonstrate additional get/set value functionality

[Lee Jones]

On Thu, 05 Dec 2024, Greg KH wrote:

> On Thu, Dec 05, 2024 at 08:38:48AM +0000, Lee Jones wrote:
> > On Wed, 04 Dec 2024, Greg KH wrote:
> > 
> > > On Wed, Dec 04, 2024 at 05:46:25PM +0000, Lee Jones wrote:
> > > > Expand the complexity of the sample driver by providing the ability to
> > > > get and set an integer.  The value is protected by a mutex.
> > > > 
> > > > Here is a simple userspace program that fully exercises the sample
> > > > driver's capabilities.
> > > > 
> > > > int main() {
> > > >   int value, new_value;
> > > >   int fd, ret;
> > > > 
> > > >   // Open the device file
> > > >   printf("Opening /dev/rust-misc-device for reading and writing\n");
> > > >   fd = open("/dev/rust-misc-device", O_RDWR);
> > > >   if (fd < 0) {
> > > >     perror("open");
> > > >     return errno;
> > > >   }
> > > > 
> > > >   // Make call into driver to say "hello"
> > > >   printf("Calling Hello\n");
> > > >   ret = ioctl(fd, RUST_MISC_DEV_HELLO, NULL);
> > > >   if (ret < 0) {
> > > >     perror("ioctl: Failed to call into Hello");
> > > >     close(fd);
> > > >     return errno;
> > > >   }
> > > > 
> > > >   // Get initial value
> > > >   printf("Fetching initial value\n");
> > > >   ret = ioctl(fd, RUST_MISC_DEV_GET_VALUE, &value);
> > > >   if (ret < 0) {
> > > >     perror("ioctl: Failed to fetch the initial value");
> > > >     close(fd);
> > > >     return errno;
> > > >   }
> > > > 
> > > >   value++;
> > > > 
> > > >   // Set value to something different
> > > >   printf("Submitting new value (%d)\n", value);
> > > >   ret = ioctl(fd, RUST_MISC_DEV_SET_VALUE, &value);
> > > >   if (ret < 0) {
> > > >     perror("ioctl: Failed to submit new value");
> > > >     close(fd);
> > > >     return errno;
> > > >   }
> > > > 
> > > >   // Ensure new value was applied
> > > >   printf("Fetching new value\n");
> > > >   ret = ioctl(fd, RUST_MISC_DEV_GET_VALUE, &new_value);
> > > >   if (ret < 0) {
> > > >     perror("ioctl: Failed to fetch the new value");
> > > >     close(fd);
> > > >     return errno;
> > > >   }
> > > > 
> > > >   if (value != new_value) {
> > > >     printf("Failed: Committed and retrieved values are different (%d - %d)\n", value, new_value);
> > > >     close(fd);
> > > >     return -1;
> > > >   }
> > > > 
> > > >   // Call the unsuccessful ioctl
> > > >   printf("Attempting to call in to an non-existent IOCTL\n");
> > > >   ret = ioctl(fd, RUST_MISC_DEV_FAIL, NULL);
> > > >   if (ret < 0) {
> > > >     perror("ioctl: Succeeded to fail - this was expected");
> > > >   } else {
> > > >     printf("ioctl: Failed to fail\n");
> > > >     close(fd);
> > > >     return -1;
> > > >   }
> > > > 
> > > >   // Close the device file
> > > >   printf("Closing /dev/rust-misc-device\n");
> > > >   close(fd);
> > > > 
> > > >   printf("Success\n");
> > > >   return 0;
> > > > }
> > > > 
> > > > Signed-off-by: Lee Jones <lee@kernel.org>
> > > > ---
> > > >  samples/rust/rust_misc_device.rs | 82 ++++++++++++++++++++++++--------
> > > >  1 file changed, 62 insertions(+), 20 deletions(-)
> > > > 
> > > > diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> > > > index 5f1b69569ef7..9c041497d881 100644
> > > > --- a/samples/rust/rust_misc_device.rs
> > > > +++ b/samples/rust/rust_misc_device.rs
> > > > @@ -2,13 +2,20 @@
> > > >  
> > > >  //! Rust misc device sample.
> > > >  
> > > > +use core::pin::Pin;
> > > > +
> > > >  use kernel::{
> > > >      c_str,
> > > > -    ioctl::_IO,
> > > > +    ioctl::{_IO, _IOC_SIZE, _IOR, _IOW},
> > > >      miscdevice::{MiscDevice, MiscDeviceOptions, MiscDeviceRegistration},
> > > > +    new_mutex,
> > > >      prelude::*,
> > > > +    sync::Mutex,
> > > > +    uaccess::{UserSlice, UserSliceReader, UserSliceWriter},
> > > >  };
> > > >  
> > > > +const RUST_MISC_DEV_GET_VALUE: u32 = _IOR::<i32>('R' as u32, 7);
> > > > +const RUST_MISC_DEV_SET_VALUE: u32 = _IOW::<i32>('R' as u32, 8);
> > > 
> > > Shouldn't this be 'W'?
> > 
> > No, I don't think so.
> > 
> > 'W' doesn't mean 'write'.  It's supposed to be a unique identifier:
> > 
> > 'W'   00-1F  linux/watchdog.h                                        conflict!
> > 'W'   00-1F  linux/wanrouter.h                                       conflict! (pre 3.9)
> > 'W'   00-3F  sound/asound.h                                          conflict!
> > 'W'   40-5F  drivers/pci/switch/switchtec.c
> > 'W'   60-61  linux/watch_queue.h
> > 
> > 'R' isn't registered for this either:
> > 
> > 'R'   00-1F  linux/random.h                                          conflict!
> > 'R'   01     linux/rfkill.h                                          conflict!
> > 'R'   20-2F  linux/trace_mmap.h
> > 'R'   C0-DF  net/bluetooth/rfcomm.h
> > 'R'   E0     uapi/linux/fsl_mc.h
> > 
> > ... but since this is just example code with no real purpose, I'm going
> > to hold short of registering a unique identifier for it.
> 
> Ah, sorry, I missed that this is the ioctl "name".  As the ptrace people
> will complain, why not use a new one?  Ick, ioctl-number.rst is way out
> of date, but I guess we should carve out one for "sample drivers, do not
> use in anything real" use cases like here.

I can carve one out for Samples.

> > > > +    fn get_value(&self, mut writer: UserSliceWriter) -> Result<isize> {
> > > > +        let guard = self.inner.lock();
> > > > +
> > > > +        pr_info!("-> Copying data to userspace (value: {})\n", &guard.value);
> > > > +
> > > > +        writer.write::<i32>(&guard.value)?;
> > > 
> > > What happens if it fails, shouldn't your pr_info() happen after this?
> > 
> > If this fails, I need the line in the log to show where it failed.
> 
> pr_info() doesn't show file lines from what I remember has that changed?
> 
> But wait, this is a misc device, you should be using dev_info() and
> friends here, no pr_*() stuff please.

Looks like no other Rust driver does this, so I would be the first.

I need to figure out how to obtain Device data via these APIs.

Can I put it on the subsequent work priority list _before_ read/write?

> > It says "copying" as in "attempting to copy", rather than "copied".
> 
> Fair enough, but if the copy fails, nothing gets printed out, right?

Not from the kernel, but userspace catches it without issue:

  ioctl: Failed to submit new value: Bad address

This does not appear to be common practice in kernel Rust.

The commonly used ? operator seems to just propagate.

-- 
Lee Jones [李琼斯]

Re: [PATCH 2/2] sample: rust_misc_device: Demonstrate additional get/set value functionality

[Greg KH]

On Thu, Dec 05, 2024 at 09:27:06AM +0000, Lee Jones wrote:
> On Thu, 05 Dec 2024, Greg KH wrote:
> 
> > On Thu, Dec 05, 2024 at 08:38:48AM +0000, Lee Jones wrote:
> > > On Wed, 04 Dec 2024, Greg KH wrote:
> > > 
> > > > On Wed, Dec 04, 2024 at 05:46:25PM +0000, Lee Jones wrote:
> > > > > Expand the complexity of the sample driver by providing the ability to
> > > > > get and set an integer.  The value is protected by a mutex.
> > > > > 
> > > > > Here is a simple userspace program that fully exercises the sample
> > > > > driver's capabilities.
> > > > > 
> > > > > int main() {
> > > > >   int value, new_value;
> > > > >   int fd, ret;
> > > > > 
> > > > >   // Open the device file
> > > > >   printf("Opening /dev/rust-misc-device for reading and writing\n");
> > > > >   fd = open("/dev/rust-misc-device", O_RDWR);
> > > > >   if (fd < 0) {
> > > > >     perror("open");
> > > > >     return errno;
> > > > >   }
> > > > > 
> > > > >   // Make call into driver to say "hello"
> > > > >   printf("Calling Hello\n");
> > > > >   ret = ioctl(fd, RUST_MISC_DEV_HELLO, NULL);
> > > > >   if (ret < 0) {
> > > > >     perror("ioctl: Failed to call into Hello");
> > > > >     close(fd);
> > > > >     return errno;
> > > > >   }
> > > > > 
> > > > >   // Get initial value
> > > > >   printf("Fetching initial value\n");
> > > > >   ret = ioctl(fd, RUST_MISC_DEV_GET_VALUE, &value);
> > > > >   if (ret < 0) {
> > > > >     perror("ioctl: Failed to fetch the initial value");
> > > > >     close(fd);
> > > > >     return errno;
> > > > >   }
> > > > > 
> > > > >   value++;
> > > > > 
> > > > >   // Set value to something different
> > > > >   printf("Submitting new value (%d)\n", value);
> > > > >   ret = ioctl(fd, RUST_MISC_DEV_SET_VALUE, &value);
> > > > >   if (ret < 0) {
> > > > >     perror("ioctl: Failed to submit new value");
> > > > >     close(fd);
> > > > >     return errno;
> > > > >   }
> > > > > 
> > > > >   // Ensure new value was applied
> > > > >   printf("Fetching new value\n");
> > > > >   ret = ioctl(fd, RUST_MISC_DEV_GET_VALUE, &new_value);
> > > > >   if (ret < 0) {
> > > > >     perror("ioctl: Failed to fetch the new value");
> > > > >     close(fd);
> > > > >     return errno;
> > > > >   }
> > > > > 
> > > > >   if (value != new_value) {
> > > > >     printf("Failed: Committed and retrieved values are different (%d - %d)\n", value, new_value);
> > > > >     close(fd);
> > > > >     return -1;
> > > > >   }
> > > > > 
> > > > >   // Call the unsuccessful ioctl
> > > > >   printf("Attempting to call in to an non-existent IOCTL\n");
> > > > >   ret = ioctl(fd, RUST_MISC_DEV_FAIL, NULL);
> > > > >   if (ret < 0) {
> > > > >     perror("ioctl: Succeeded to fail - this was expected");
> > > > >   } else {
> > > > >     printf("ioctl: Failed to fail\n");
> > > > >     close(fd);
> > > > >     return -1;
> > > > >   }
> > > > > 
> > > > >   // Close the device file
> > > > >   printf("Closing /dev/rust-misc-device\n");
> > > > >   close(fd);
> > > > > 
> > > > >   printf("Success\n");
> > > > >   return 0;
> > > > > }
> > > > > 
> > > > > Signed-off-by: Lee Jones <lee@kernel.org>
> > > > > ---
> > > > >  samples/rust/rust_misc_device.rs | 82 ++++++++++++++++++++++++--------
> > > > >  1 file changed, 62 insertions(+), 20 deletions(-)
> > > > > 
> > > > > diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> > > > > index 5f1b69569ef7..9c041497d881 100644
> > > > > --- a/samples/rust/rust_misc_device.rs
> > > > > +++ b/samples/rust/rust_misc_device.rs
> > > > > @@ -2,13 +2,20 @@
> > > > >  
> > > > >  //! Rust misc device sample.
> > > > >  
> > > > > +use core::pin::Pin;
> > > > > +
> > > > >  use kernel::{
> > > > >      c_str,
> > > > > -    ioctl::_IO,
> > > > > +    ioctl::{_IO, _IOC_SIZE, _IOR, _IOW},
> > > > >      miscdevice::{MiscDevice, MiscDeviceOptions, MiscDeviceRegistration},
> > > > > +    new_mutex,
> > > > >      prelude::*,
> > > > > +    sync::Mutex,
> > > > > +    uaccess::{UserSlice, UserSliceReader, UserSliceWriter},
> > > > >  };
> > > > >  
> > > > > +const RUST_MISC_DEV_GET_VALUE: u32 = _IOR::<i32>('R' as u32, 7);
> > > > > +const RUST_MISC_DEV_SET_VALUE: u32 = _IOW::<i32>('R' as u32, 8);
> > > > 
> > > > Shouldn't this be 'W'?
> > > 
> > > No, I don't think so.
> > > 
> > > 'W' doesn't mean 'write'.  It's supposed to be a unique identifier:
> > > 
> > > 'W'   00-1F  linux/watchdog.h                                        conflict!
> > > 'W'   00-1F  linux/wanrouter.h                                       conflict! (pre 3.9)
> > > 'W'   00-3F  sound/asound.h                                          conflict!
> > > 'W'   40-5F  drivers/pci/switch/switchtec.c
> > > 'W'   60-61  linux/watch_queue.h
> > > 
> > > 'R' isn't registered for this either:
> > > 
> > > 'R'   00-1F  linux/random.h                                          conflict!
> > > 'R'   01     linux/rfkill.h                                          conflict!
> > > 'R'   20-2F  linux/trace_mmap.h
> > > 'R'   C0-DF  net/bluetooth/rfcomm.h
> > > 'R'   E0     uapi/linux/fsl_mc.h
> > > 
> > > ... but since this is just example code with no real purpose, I'm going
> > > to hold short of registering a unique identifier for it.
> > 
> > Ah, sorry, I missed that this is the ioctl "name".  As the ptrace people
> > will complain, why not use a new one?  Ick, ioctl-number.rst is way out
> > of date, but I guess we should carve out one for "sample drivers, do not
> > use in anything real" use cases like here.
> 
> I can carve one out for Samples.
> 
> > > > > +    fn get_value(&self, mut writer: UserSliceWriter) -> Result<isize> {
> > > > > +        let guard = self.inner.lock();
> > > > > +
> > > > > +        pr_info!("-> Copying data to userspace (value: {})\n", &guard.value);
> > > > > +
> > > > > +        writer.write::<i32>(&guard.value)?;
> > > > 
> > > > What happens if it fails, shouldn't your pr_info() happen after this?
> > > 
> > > If this fails, I need the line in the log to show where it failed.
> > 
> > pr_info() doesn't show file lines from what I remember has that changed?
> > 
> > But wait, this is a misc device, you should be using dev_info() and
> > friends here, no pr_*() stuff please.
> 
> Looks like no other Rust driver does this, so I would be the first.

Yes, because those macros just got added this release cycle :)

Also, moving the existing in-tree drivers to use these macros would be a
good idea.

> I need to figure out how to obtain Device data via these APIs.

You should have access to it from the misc device structure.

> Can I put it on the subsequent work priority list _before_ read/write?

It should be in patch 1 :)

> > > It says "copying" as in "attempting to copy", rather than "copied".
> > 
> > Fair enough, but if the copy fails, nothing gets printed out, right?
> 
> Not from the kernel, but userspace catches it without issue:
> 
>   ioctl: Failed to submit new value: Bad address
> 
> This does not appear to be common practice in kernel Rust.
> 
> The commonly used ? operator seems to just propagate.

Ok, nice!

automatic error handling is going to be one of the killer features here.

greg k-h

Re: [PATCH 2/2] sample: rust_misc_device: Demonstrate additional get/set value functionality

[Miguel Ojeda]

On Wed, Dec 4, 2024 at 6:46 PM Lee Jones <lee@kernel.org> wrote:
>
> Here is a simple userspace program that fully exercises the sample
> driver's capabilities.

Before I forget: one idea would be nice to get this program into an
actual file so that people can easily give it a try. Or maybe as a
```c ...``` code block in the docs.

Thanks!

Cheers,
Miguel

Re: [PATCH 1/2] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Greg KH]

On Wed, Dec 04, 2024 at 05:46:24PM +0000, Lee Jones wrote:
> This sample driver demonstrates the following basic operations:
> 
> * Register a Misc Device
> * Create /dev/rust-misc-device
> * Open the aforementioned character device
> * Operate on the character device via a simple ioctl()
> * Close the character device
> 
> Signed-off-by: Lee Jones <lee@kernel.org>
> ---
>  samples/rust/Kconfig             | 10 ++++
>  samples/rust/Makefile            |  1 +
>  samples/rust/rust_misc_device.rs | 84 ++++++++++++++++++++++++++++++++
>  3 files changed, 95 insertions(+)
>  create mode 100644 samples/rust/rust_misc_device.rs
> 
> diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> index b0f74a81c8f9..df384e679901 100644
> --- a/samples/rust/Kconfig
> +++ b/samples/rust/Kconfig
> @@ -20,6 +20,16 @@ config SAMPLE_RUST_MINIMAL
>  
>  	  If unsure, say N.
>  
> +config SAMPLE_RUST_MISC_DEVICE
> +	tristate "Misc device"
> +	help
> +	  This option builds the Rust misc device.
> +
> +	  To compile this as a module, choose M here:
> +	  the module will be called rust_misc_device.
> +
> +	  If unsure, say N.
> +
>  config SAMPLE_RUST_PRINT
>  	tristate "Printing macros"
>  	help
> diff --git a/samples/rust/Makefile b/samples/rust/Makefile
> index c1a5c1655395..ad4b97a98580 100644
> --- a/samples/rust/Makefile
> +++ b/samples/rust/Makefile
> @@ -2,6 +2,7 @@
>  ccflags-y += -I$(src)				# needed for trace events
>  
>  obj-$(CONFIG_SAMPLE_RUST_MINIMAL)		+= rust_minimal.o
> +obj-$(CONFIG_SAMPLE_RUST_MISC_DEVICE)		+= rust_misc_device.o
>  obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
>  
>  rust_print-y := rust_print_main.o rust_print_events.o
> diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> new file mode 100644
> index 000000000000..5f1b69569ef7
> --- /dev/null
> +++ b/samples/rust/rust_misc_device.rs
> @@ -0,0 +1,84 @@
> +// SPDX-License-Identifier: GPL-2.0

Nit, you forgot a copyright line here :)

> +
> +//! Rust misc device sample.
> +
> +use kernel::{
> +    c_str,
> +    ioctl::_IO,
> +    miscdevice::{MiscDevice, MiscDeviceOptions, MiscDeviceRegistration},
> +    prelude::*,
> +};
> +
> +const RUST_MISC_DEV_HELLO: u32 = _IO('R' as u32, 9);
> +
> +module! {
> +    type: RustMiscDeviceModule,
> +    name: "rust_misc_device",
> +    author: "Lee Jones",
> +    description: "Rust misc device sample",
> +    license: "GPL",
> +}
> +
> +struct RustMiscDeviceModule {
> +    _miscdev: Pin<KBox<MiscDeviceRegistration<RustMiscDevice>>>,
> +}
> +
> +impl kernel::Module for RustMiscDeviceModule {
> +    fn init(_module: &'static ThisModule) -> Result<Self> {
> +        pr_info!("Initialising Rust Misc Device Sample\n");
> +
> +        let options = MiscDeviceOptions {
> +            name: c_str!("rust-misc-device"),
> +        };
> +
> +        Ok(Self {
> +            _miscdev: KBox::pin_init(
> +                MiscDeviceRegistration::<RustMiscDevice>::register(options),
> +                GFP_KERNEL,
> +            )?,
> +        })
> +    }
> +}
> +
> +struct RustMiscDevice;
> +
> +impl RustMiscDevice {
> +    fn new() -> Self {
> +        Self
> +    }
> +}
> +
> +#[vtable]
> +impl MiscDevice for RustMiscDevice {
> +    type Ptr = KBox<Self>;
> +
> +    fn open() -> Result<KBox<Self>> {
> +        pr_info!("Opening Rust Misc Device Sample\n");
> +
> +        Ok(KBox::new(RustMiscDevice::new(), GFP_KERNEL)?)
> +    }
> +
> +    fn ioctl(
> +        _device: <Self::Ptr as kernel::types::ForeignOwnable>::Borrowed<'_>,
> +        cmd: u32,
> +        _arg: usize,
> +    ) -> Result<isize> {
> +        pr_info!("IOCTLing Rust Misc Device Sample\n");
> +
> +        match cmd {
> +            RUST_MISC_DEV_HELLO => pr_info!("Hello from the Rust Misc Device\n"),
> +            _ => {
> +                pr_err!("IOCTL not recognised: {}\n", cmd);
> +                return Err(EINVAL);
> +            }
> +        }
> +
> +        Ok(0)
> +    }
> +}
> +
> +impl Drop for RustMiscDevice {
> +    fn drop(&mut self) {
> +        pr_info!("Exiting the Rust Misc Device Sample\n");
> +    }

Cool!  But no read/write functionality?  :)

Anyway, other than the copyright, this looks good to me.

Although we should get the "validate the data" rust patch set in here
soon, so we don't have to go and fix up all users of the miscdev rust
api at once.  Maybe I'll dig that series up over the holiday break if
someone doesn't beat me to it.

thanks,

greg k-h

Re: [PATCH 1/2] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Lee Jones]

On Wed, 04 Dec 2024, Greg KH wrote:

> On Wed, Dec 04, 2024 at 05:46:24PM +0000, Lee Jones wrote:
> > This sample driver demonstrates the following basic operations:
> > 
> > * Register a Misc Device
> > * Create /dev/rust-misc-device
> > * Open the aforementioned character device
> > * Operate on the character device via a simple ioctl()
> > * Close the character device
> > 
> > Signed-off-by: Lee Jones <lee@kernel.org>
> > ---
> >  samples/rust/Kconfig             | 10 ++++
> >  samples/rust/Makefile            |  1 +
> >  samples/rust/rust_misc_device.rs | 84 ++++++++++++++++++++++++++++++++
> >  3 files changed, 95 insertions(+)
> >  create mode 100644 samples/rust/rust_misc_device.rs
> > 
> > diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> > index b0f74a81c8f9..df384e679901 100644
> > --- a/samples/rust/Kconfig
> > +++ b/samples/rust/Kconfig
> > @@ -20,6 +20,16 @@ config SAMPLE_RUST_MINIMAL
> >  
> >  	  If unsure, say N.
> >  
> > +config SAMPLE_RUST_MISC_DEVICE
> > +	tristate "Misc device"
> > +	help
> > +	  This option builds the Rust misc device.
> > +
> > +	  To compile this as a module, choose M here:
> > +	  the module will be called rust_misc_device.
> > +
> > +	  If unsure, say N.
> > +
> >  config SAMPLE_RUST_PRINT
> >  	tristate "Printing macros"
> >  	help
> > diff --git a/samples/rust/Makefile b/samples/rust/Makefile
> > index c1a5c1655395..ad4b97a98580 100644
> > --- a/samples/rust/Makefile
> > +++ b/samples/rust/Makefile
> > @@ -2,6 +2,7 @@
> >  ccflags-y += -I$(src)				# needed for trace events
> >  
> >  obj-$(CONFIG_SAMPLE_RUST_MINIMAL)		+= rust_minimal.o
> > +obj-$(CONFIG_SAMPLE_RUST_MISC_DEVICE)		+= rust_misc_device.o
> >  obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
> >  
> >  rust_print-y := rust_print_main.o rust_print_events.o
> > diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> > new file mode 100644
> > index 000000000000..5f1b69569ef7
> > --- /dev/null
> > +++ b/samples/rust/rust_misc_device.rs
> > @@ -0,0 +1,84 @@
> > +// SPDX-License-Identifier: GPL-2.0
> 
> Nit, you forgot a copyright line here :)

I can add one, but none of the other drivers in this directory has one.

>> > +
> > +//! Rust misc device sample.
> > +
> > +use kernel::{
> > +    c_str,
> > +    ioctl::_IO,
> > +    miscdevice::{MiscDevice, MiscDeviceOptions, MiscDeviceRegistration},
> > +    prelude::*,
> > +};
> > +
> > +const RUST_MISC_DEV_HELLO: u32 = _IO('R' as u32, 9);
> > +
> > +module! {
> > +    type: RustMiscDeviceModule,
> > +    name: "rust_misc_device",
> > +    author: "Lee Jones",
> > +    description: "Rust misc device sample",
> > +    license: "GPL",
> > +}
> > +
> > +struct RustMiscDeviceModule {
> > +    _miscdev: Pin<KBox<MiscDeviceRegistration<RustMiscDevice>>>,
> > +}
> > +
> > +impl kernel::Module for RustMiscDeviceModule {
> > +    fn init(_module: &'static ThisModule) -> Result<Self> {
> > +        pr_info!("Initialising Rust Misc Device Sample\n");
> > +
> > +        let options = MiscDeviceOptions {
> > +            name: c_str!("rust-misc-device"),
> > +        };
> > +
> > +        Ok(Self {
> > +            _miscdev: KBox::pin_init(
> > +                MiscDeviceRegistration::<RustMiscDevice>::register(options),
> > +                GFP_KERNEL,
> > +            )?,
> > +        })
> > +    }
> > +}
> > +
> > +struct RustMiscDevice;
> > +
> > +impl RustMiscDevice {
> > +    fn new() -> Self {
> > +        Self
> > +    }
> > +}
> > +
> > +#[vtable]
> > +impl MiscDevice for RustMiscDevice {
> > +    type Ptr = KBox<Self>;
> > +
> > +    fn open() -> Result<KBox<Self>> {
> > +        pr_info!("Opening Rust Misc Device Sample\n");
> > +
> > +        Ok(KBox::new(RustMiscDevice::new(), GFP_KERNEL)?)
> > +    }
> > +
> > +    fn ioctl(
> > +        _device: <Self::Ptr as kernel::types::ForeignOwnable>::Borrowed<'_>,
> > +        cmd: u32,
> > +        _arg: usize,
> > +    ) -> Result<isize> {
> > +        pr_info!("IOCTLing Rust Misc Device Sample\n");
> > +
> > +        match cmd {
> > +            RUST_MISC_DEV_HELLO => pr_info!("Hello from the Rust Misc Device\n"),
> > +            _ => {
> > +                pr_err!("IOCTL not recognised: {}\n", cmd);
> > +                return Err(EINVAL);
> > +            }
> > +        }
> > +
> > +        Ok(0)
> > +    }
> > +}
> > +
> > +impl Drop for RustMiscDevice {
> > +    fn drop(&mut self) {
> > +        pr_info!("Exiting the Rust Misc Device Sample\n");
> > +    }
> 
> Cool!  But no read/write functionality?  :)

Happy to add it subsequently.

> Anyway, other than the copyright, this looks good to me.
> 
> Although we should get the "validate the data" rust patch set in here
> soon, so we don't have to go and fix up all users of the miscdev rust
> api at once.  Maybe I'll dig that series up over the holiday break if
> someone doesn't beat me to it.

What needs doing?  Do you have a link?

-- 
Lee Jones [李琼斯]

Re: [PATCH 1/2] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Greg KH]

On Thu, Dec 05, 2024 at 08:41:01AM +0000, Lee Jones wrote:
> On Wed, 04 Dec 2024, Greg KH wrote:
> 
> > On Wed, Dec 04, 2024 at 05:46:24PM +0000, Lee Jones wrote:
> > > This sample driver demonstrates the following basic operations:
> > > 
> > > * Register a Misc Device
> > > * Create /dev/rust-misc-device
> > > * Open the aforementioned character device
> > > * Operate on the character device via a simple ioctl()
> > > * Close the character device
> > > 
> > > Signed-off-by: Lee Jones <lee@kernel.org>
> > > ---
> > >  samples/rust/Kconfig             | 10 ++++
> > >  samples/rust/Makefile            |  1 +
> > >  samples/rust/rust_misc_device.rs | 84 ++++++++++++++++++++++++++++++++
> > >  3 files changed, 95 insertions(+)
> > >  create mode 100644 samples/rust/rust_misc_device.rs
> > > 
> > > diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> > > index b0f74a81c8f9..df384e679901 100644
> > > --- a/samples/rust/Kconfig
> > > +++ b/samples/rust/Kconfig
> > > @@ -20,6 +20,16 @@ config SAMPLE_RUST_MINIMAL
> > >  
> > >  	  If unsure, say N.
> > >  
> > > +config SAMPLE_RUST_MISC_DEVICE
> > > +	tristate "Misc device"
> > > +	help
> > > +	  This option builds the Rust misc device.
> > > +
> > > +	  To compile this as a module, choose M here:
> > > +	  the module will be called rust_misc_device.
> > > +
> > > +	  If unsure, say N.
> > > +
> > >  config SAMPLE_RUST_PRINT
> > >  	tristate "Printing macros"
> > >  	help
> > > diff --git a/samples/rust/Makefile b/samples/rust/Makefile
> > > index c1a5c1655395..ad4b97a98580 100644
> > > --- a/samples/rust/Makefile
> > > +++ b/samples/rust/Makefile
> > > @@ -2,6 +2,7 @@
> > >  ccflags-y += -I$(src)				# needed for trace events
> > >  
> > >  obj-$(CONFIG_SAMPLE_RUST_MINIMAL)		+= rust_minimal.o
> > > +obj-$(CONFIG_SAMPLE_RUST_MISC_DEVICE)		+= rust_misc_device.o
> > >  obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
> > >  
> > >  rust_print-y := rust_print_main.o rust_print_events.o
> > > diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> > > new file mode 100644
> > > index 000000000000..5f1b69569ef7
> > > --- /dev/null
> > > +++ b/samples/rust/rust_misc_device.rs
> > > @@ -0,0 +1,84 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > 
> > Nit, you forgot a copyright line here :)
> 
> I can add one, but none of the other drivers in this directory has one.

I think the copyright owner of this file will appreciate that.  In fact,
I think it might be required by them :)

> > Anyway, other than the copyright, this looks good to me.
> > 
> > Although we should get the "validate the data" rust patch set in here
> > soon, so we don't have to go and fix up all users of the miscdev rust
> > api at once.  Maybe I'll dig that series up over the holiday break if
> > someone doesn't beat me to it.
> 
> What needs doing?  Do you have a link?

https://lore.kernel.org/r/20240925205244.873020-1-benno.lossin@proton.me

But in thinking about it more, this isn't going to work well with misc
devices as the data is coming from userspace, which already goes through
the user slice code.  Unless userslice should be marking the data as
untrusted?  I think that needs to happen as well.

thanks,

greg k-h

Re: [PATCH 1/2] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Lee Jones]

On Thu, 05 Dec 2024, Greg KH wrote:

> On Thu, Dec 05, 2024 at 08:41:01AM +0000, Lee Jones wrote:
> > On Wed, 04 Dec 2024, Greg KH wrote:
> > 
> > > On Wed, Dec 04, 2024 at 05:46:24PM +0000, Lee Jones wrote:
> > > > This sample driver demonstrates the following basic operations:
> > > > 
> > > > * Register a Misc Device
> > > > * Create /dev/rust-misc-device
> > > > * Open the aforementioned character device
> > > > * Operate on the character device via a simple ioctl()
> > > > * Close the character device
> > > > 
> > > > Signed-off-by: Lee Jones <lee@kernel.org>
> > > > ---
> > > >  samples/rust/Kconfig             | 10 ++++
> > > >  samples/rust/Makefile            |  1 +
> > > >  samples/rust/rust_misc_device.rs | 84 ++++++++++++++++++++++++++++++++
> > > >  3 files changed, 95 insertions(+)
> > > >  create mode 100644 samples/rust/rust_misc_device.rs
> > > > 
> > > > diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> > > > index b0f74a81c8f9..df384e679901 100644
> > > > --- a/samples/rust/Kconfig
> > > > +++ b/samples/rust/Kconfig
> > > > @@ -20,6 +20,16 @@ config SAMPLE_RUST_MINIMAL
> > > >  
> > > >  	  If unsure, say N.
> > > >  
> > > > +config SAMPLE_RUST_MISC_DEVICE
> > > > +	tristate "Misc device"
> > > > +	help
> > > > +	  This option builds the Rust misc device.
> > > > +
> > > > +	  To compile this as a module, choose M here:
> > > > +	  the module will be called rust_misc_device.
> > > > +
> > > > +	  If unsure, say N.
> > > > +
> > > >  config SAMPLE_RUST_PRINT
> > > >  	tristate "Printing macros"
> > > >  	help
> > > > diff --git a/samples/rust/Makefile b/samples/rust/Makefile
> > > > index c1a5c1655395..ad4b97a98580 100644
> > > > --- a/samples/rust/Makefile
> > > > +++ b/samples/rust/Makefile
> > > > @@ -2,6 +2,7 @@
> > > >  ccflags-y += -I$(src)				# needed for trace events
> > > >  
> > > >  obj-$(CONFIG_SAMPLE_RUST_MINIMAL)		+= rust_minimal.o
> > > > +obj-$(CONFIG_SAMPLE_RUST_MISC_DEVICE)		+= rust_misc_device.o
> > > >  obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
> > > >  
> > > >  rust_print-y := rust_print_main.o rust_print_events.o
> > > > diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> > > > new file mode 100644
> > > > index 000000000000..5f1b69569ef7
> > > > --- /dev/null
> > > > +++ b/samples/rust/rust_misc_device.rs
> > > > @@ -0,0 +1,84 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > 
> > > Nit, you forgot a copyright line here :)
> > 
> > I can add one, but none of the other drivers in this directory has one.
> 
> I think the copyright owner of this file will appreciate that.  In fact,
> I think it might be required by them :)

I think you're right.  Probably just an oversight from the original.

> > > Anyway, other than the copyright, this looks good to me.
> > > 
> > > Although we should get the "validate the data" rust patch set in here
> > > soon, so we don't have to go and fix up all users of the miscdev rust
> > > api at once.  Maybe I'll dig that series up over the holiday break if
> > > someone doesn't beat me to it.
> > 
> > What needs doing?  Do you have a link?
> 
> https://lore.kernel.org/r/20240925205244.873020-1-benno.lossin@proton.me
> 
> But in thinking about it more, this isn't going to work well with misc
> devices as the data is coming from userspace, which already goes through
> the user slice code.  Unless userslice should be marking the data as
> untrusted?  I think that needs to happen as well.

Not too relevant here I think.  The data coming in is just a single int
that is used as-is rather than any kind of index into memory.

-- 
Lee Jones [李琼斯]

Re: [PATCH 1/2] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Greg KH]

On Thu, Dec 05, 2024 at 09:17:04AM +0000, Lee Jones wrote:
> On Thu, 05 Dec 2024, Greg KH wrote:
> 
> > On Thu, Dec 05, 2024 at 08:41:01AM +0000, Lee Jones wrote:
> > > On Wed, 04 Dec 2024, Greg KH wrote:
> > > 
> > > > On Wed, Dec 04, 2024 at 05:46:24PM +0000, Lee Jones wrote:
> > > > > This sample driver demonstrates the following basic operations:
> > > > > 
> > > > > * Register a Misc Device
> > > > > * Create /dev/rust-misc-device
> > > > > * Open the aforementioned character device
> > > > > * Operate on the character device via a simple ioctl()
> > > > > * Close the character device
> > > > > 
> > > > > Signed-off-by: Lee Jones <lee@kernel.org>
> > > > > ---
> > > > >  samples/rust/Kconfig             | 10 ++++
> > > > >  samples/rust/Makefile            |  1 +
> > > > >  samples/rust/rust_misc_device.rs | 84 ++++++++++++++++++++++++++++++++
> > > > >  3 files changed, 95 insertions(+)
> > > > >  create mode 100644 samples/rust/rust_misc_device.rs
> > > > > 
> > > > > diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> > > > > index b0f74a81c8f9..df384e679901 100644
> > > > > --- a/samples/rust/Kconfig
> > > > > +++ b/samples/rust/Kconfig
> > > > > @@ -20,6 +20,16 @@ config SAMPLE_RUST_MINIMAL
> > > > >  
> > > > >  	  If unsure, say N.
> > > > >  
> > > > > +config SAMPLE_RUST_MISC_DEVICE
> > > > > +	tristate "Misc device"
> > > > > +	help
> > > > > +	  This option builds the Rust misc device.
> > > > > +
> > > > > +	  To compile this as a module, choose M here:
> > > > > +	  the module will be called rust_misc_device.
> > > > > +
> > > > > +	  If unsure, say N.
> > > > > +
> > > > >  config SAMPLE_RUST_PRINT
> > > > >  	tristate "Printing macros"
> > > > >  	help
> > > > > diff --git a/samples/rust/Makefile b/samples/rust/Makefile
> > > > > index c1a5c1655395..ad4b97a98580 100644
> > > > > --- a/samples/rust/Makefile
> > > > > +++ b/samples/rust/Makefile
> > > > > @@ -2,6 +2,7 @@
> > > > >  ccflags-y += -I$(src)				# needed for trace events
> > > > >  
> > > > >  obj-$(CONFIG_SAMPLE_RUST_MINIMAL)		+= rust_minimal.o
> > > > > +obj-$(CONFIG_SAMPLE_RUST_MISC_DEVICE)		+= rust_misc_device.o
> > > > >  obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
> > > > >  
> > > > >  rust_print-y := rust_print_main.o rust_print_events.o
> > > > > diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> > > > > new file mode 100644
> > > > > index 000000000000..5f1b69569ef7
> > > > > --- /dev/null
> > > > > +++ b/samples/rust/rust_misc_device.rs
> > > > > @@ -0,0 +1,84 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > 
> > > > Nit, you forgot a copyright line here :)
> > > 
> > > I can add one, but none of the other drivers in this directory has one.
> > 
> > I think the copyright owner of this file will appreciate that.  In fact,
> > I think it might be required by them :)
> 
> I think you're right.  Probably just an oversight from the original.
> 
> > > > Anyway, other than the copyright, this looks good to me.
> > > > 
> > > > Although we should get the "validate the data" rust patch set in here
> > > > soon, so we don't have to go and fix up all users of the miscdev rust
> > > > api at once.  Maybe I'll dig that series up over the holiday break if
> > > > someone doesn't beat me to it.
> > > 
> > > What needs doing?  Do you have a link?
> > 
> > https://lore.kernel.org/r/20240925205244.873020-1-benno.lossin@proton.me
> > 
> > But in thinking about it more, this isn't going to work well with misc
> > devices as the data is coming from userspace, which already goes through
> > the user slice code.  Unless userslice should be marking the data as
> > untrusted?  I think that needs to happen as well.
> 
> Not too relevant here I think.  The data coming in is just a single int
> that is used as-is rather than any kind of index into memory.

For now, yes.  But in reality, the data is "untrusted" as userspace
provided it, and for a sample, we should at least document that
otherwise people will forget.

thanks,

greg k-h

Re: [PATCH 1/2] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Benoît du Garreau]

On Wed, 4 Dec 2024 17:46:24 +0000 Lee Jones <lee@kernel.org> wrote:

> This sample driver demonstrates the following basic operations:
>
> * Register a Misc Device
> * Create /dev/rust-misc-device
> * Open the aforementioned character device
> * Operate on the character device via a simple ioctl()
> * Close the character device
>
> Signed-off-by: Lee Jones <lee@kernel.org>
> ---
>  samples/rust/Kconfig             | 10 ++++
>  samples/rust/Makefile            |  1 +
>  samples/rust/rust_misc_device.rs | 84 ++++++++++++++++++++++++++++++++
>  3 files changed, 95 insertions(+)
>  create mode 100644 samples/rust/rust_misc_device.rs
>
> diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> index b0f74a81c8f9..df384e679901 100644
> --- a/samples/rust/Kconfig
> +++ b/samples/rust/Kconfig
> @@ -20,6 +20,16 @@ config SAMPLE_RUST_MINIMAL
>
>  	  If unsure, say N.
>
> +config SAMPLE_RUST_MISC_DEVICE
> +	tristate "Misc device"
> +	help
> +	  This option builds the Rust misc device.
> +
> +	  To compile this as a module, choose M here:
> +	  the module will be called rust_misc_device.
> +
> +	  If unsure, say N.
> +
>  config SAMPLE_RUST_PRINT
>  	tristate "Printing macros"
>  	help
> diff --git a/samples/rust/Makefile b/samples/rust/Makefile
> index c1a5c1655395..ad4b97a98580 100644
> --- a/samples/rust/Makefile
> +++ b/samples/rust/Makefile
> @@ -2,6 +2,7 @@
>  ccflags-y += -I$(src)				# needed for trace events
>
>  obj-$(CONFIG_SAMPLE_RUST_MINIMAL)		+= rust_minimal.o
> +obj-$(CONFIG_SAMPLE_RUST_MISC_DEVICE)		+= rust_misc_device.o
>  obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
>
>  rust_print-y := rust_print_main.o rust_print_events.o
> diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> new file mode 100644
> index 000000000000..5f1b69569ef7
> --- /dev/null
> +++ b/samples/rust/rust_misc_device.rs
> @@ -0,0 +1,84 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Rust misc device sample.
> +
> +use kernel::{
> +    c_str,
> +    ioctl::_IO,
> +    miscdevice::{MiscDevice, MiscDeviceOptions, MiscDeviceRegistration},
> +    prelude::*,
> +};
> +
> +const RUST_MISC_DEV_HELLO: u32 = _IO('R' as u32, 9);
> +
> +module! {
> +    type: RustMiscDeviceModule,
> +    name: "rust_misc_device",
> +    author: "Lee Jones",
> +    description: "Rust misc device sample",
> +    license: "GPL",
> +}
> +
> +struct RustMiscDeviceModule {
> +    _miscdev: Pin<KBox<MiscDeviceRegistration<RustMiscDevice>>>,
> +}
> +
> +impl kernel::Module for RustMiscDeviceModule {

This could probably use `kernel::InPlaceModule` and avoid allocating
the registration.

> +    fn init(_module: &'static ThisModule) -> Result<Self> {
> +        pr_info!("Initialising Rust Misc Device Sample\n");
> +
> +        let options = MiscDeviceOptions {
> +            name: c_str!("rust-misc-device"),
> +        };
> +
> +        Ok(Self {
> +            _miscdev: KBox::pin_init(
> +                MiscDeviceRegistration::<RustMiscDevice>::register(options),
> +                GFP_KERNEL,
> +            )?,
> +        })
> +    }
> +}
> +
> +struct RustMiscDevice;
> +
> +impl RustMiscDevice {
> +    fn new() -> Self {
> +        Self
> +    }
> +}

Given that this function is nearly empty and removed in the next commit,
it could probably not exist in the first place.

> +
> +#[vtable]
> +impl MiscDevice for RustMiscDevice {
> +    type Ptr = KBox<Self>;
> +
> +    fn open() -> Result<KBox<Self>> {
> +        pr_info!("Opening Rust Misc Device Sample\n");
> +
> +        Ok(KBox::new(RustMiscDevice::new(), GFP_KERNEL)?)
> +    }
> +
> +    fn ioctl(
> +        _device: <Self::Ptr as kernel::types::ForeignOwnable>::Borrowed<'_>,
> +        cmd: u32,
> +        _arg: usize,
> +    ) -> Result<isize> {
> +        pr_info!("IOCTLing Rust Misc Device Sample\n");
> +
> +        match cmd {
> +            RUST_MISC_DEV_HELLO => pr_info!("Hello from the Rust Misc Device\n"),
> +            _ => {
> +                pr_err!("IOCTL not recognised: {}\n", cmd);
> +                return Err(EINVAL);
> +            }
> +        }
> +
> +        Ok(0)
> +    }
> +}
> +
> +impl Drop for RustMiscDevice {
> +    fn drop(&mut self) {
> +        pr_info!("Exiting the Rust Misc Device Sample\n");
> +    }
> +}
> --
> 2.47.0.338.g60cca15819-goog


Benoît du Garreau

Re: [PATCH 1/2] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Lee Jones]

On Thu, 05 Dec 2024, Benoît du Garreau wrote:

> On Wed, 4 Dec 2024 17:46:24 +0000 Lee Jones <lee@kernel.org> wrote:
> 
> > This sample driver demonstrates the following basic operations:
> >
> > * Register a Misc Device
> > * Create /dev/rust-misc-device
> > * Open the aforementioned character device
> > * Operate on the character device via a simple ioctl()
> > * Close the character device
> >
> > Signed-off-by: Lee Jones <lee@kernel.org>
> > ---
> >  samples/rust/Kconfig             | 10 ++++
> >  samples/rust/Makefile            |  1 +
> >  samples/rust/rust_misc_device.rs | 84 ++++++++++++++++++++++++++++++++
> >  3 files changed, 95 insertions(+)
> >  create mode 100644 samples/rust/rust_misc_device.rs
> >
> > diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> > index b0f74a81c8f9..df384e679901 100644
> > --- a/samples/rust/Kconfig
> > +++ b/samples/rust/Kconfig
> > @@ -20,6 +20,16 @@ config SAMPLE_RUST_MINIMAL
> >
> >  	  If unsure, say N.
> >
> > +config SAMPLE_RUST_MISC_DEVICE
> > +	tristate "Misc device"
> > +	help
> > +	  This option builds the Rust misc device.
> > +
> > +	  To compile this as a module, choose M here:
> > +	  the module will be called rust_misc_device.
> > +
> > +	  If unsure, say N.
> > +
> >  config SAMPLE_RUST_PRINT
> >  	tristate "Printing macros"
> >  	help
> > diff --git a/samples/rust/Makefile b/samples/rust/Makefile
> > index c1a5c1655395..ad4b97a98580 100644
> > --- a/samples/rust/Makefile
> > +++ b/samples/rust/Makefile
> > @@ -2,6 +2,7 @@
> >  ccflags-y += -I$(src)				# needed for trace events
> >
> >  obj-$(CONFIG_SAMPLE_RUST_MINIMAL)		+= rust_minimal.o
> > +obj-$(CONFIG_SAMPLE_RUST_MISC_DEVICE)		+= rust_misc_device.o
> >  obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
> >
> >  rust_print-y := rust_print_main.o rust_print_events.o
> > diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> > new file mode 100644
> > index 000000000000..5f1b69569ef7
> > --- /dev/null
> > +++ b/samples/rust/rust_misc_device.rs
> > @@ -0,0 +1,84 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Rust misc device sample.
> > +
> > +use kernel::{
> > +    c_str,
> > +    ioctl::_IO,
> > +    miscdevice::{MiscDevice, MiscDeviceOptions, MiscDeviceRegistration},
> > +    prelude::*,
> > +};
> > +
> > +const RUST_MISC_DEV_HELLO: u32 = _IO('R' as u32, 9);
> > +
> > +module! {
> > +    type: RustMiscDeviceModule,
> > +    name: "rust_misc_device",
> > +    author: "Lee Jones",
> > +    description: "Rust misc device sample",
> > +    license: "GPL",
> > +}
> > +
> > +struct RustMiscDeviceModule {
> > +    _miscdev: Pin<KBox<MiscDeviceRegistration<RustMiscDevice>>>,
> > +}
> > +
> > +impl kernel::Module for RustMiscDeviceModule {
> 
> This could probably use `kernel::InPlaceModule` and avoid allocating
> the registration.

Is there a way to do this and keep the print?

> > +    fn init(_module: &'static ThisModule) -> Result<Self> {
> > +        pr_info!("Initialising Rust Misc Device Sample\n");
> > +
> > +        let options = MiscDeviceOptions {
> > +            name: c_str!("rust-misc-device"),
> > +        };
> > +
> > +        Ok(Self {
> > +            _miscdev: KBox::pin_init(
> > +                MiscDeviceRegistration::<RustMiscDevice>::register(options),
> > +                GFP_KERNEL,
> > +            )?,
> > +        })
> > +    }
> > +}
> > +
> > +struct RustMiscDevice;
> > +
> > +impl RustMiscDevice {
> > +    fn new() -> Self {
> > +        Self
> > +    }
> > +}
> 
> Given that this function is nearly empty and removed in the next commit,
> it could probably not exist in the first place.

This is showing my Rust immaturity here.

How does one create a new Self without new() in this context?

> > +
> > +#[vtable]
> > +impl MiscDevice for RustMiscDevice {
> > +    type Ptr = KBox<Self>;
> > +
> > +    fn open() -> Result<KBox<Self>> {
> > +        pr_info!("Opening Rust Misc Device Sample\n");
> > +
> > +        Ok(KBox::new(RustMiscDevice::new(), GFP_KERNEL)?)
> > +    }
> > +
> > +    fn ioctl(
> > +        _device: <Self::Ptr as kernel::types::ForeignOwnable>::Borrowed<'_>,
> > +        cmd: u32,
> > +        _arg: usize,
> > +    ) -> Result<isize> {
> > +        pr_info!("IOCTLing Rust Misc Device Sample\n");
> > +
> > +        match cmd {
> > +            RUST_MISC_DEV_HELLO => pr_info!("Hello from the Rust Misc Device\n"),
> > +            _ => {
> > +                pr_err!("IOCTL not recognised: {}\n", cmd);
> > +                return Err(EINVAL);
> > +            }
> > +        }
> > +
> > +        Ok(0)
> > +    }
> > +}
> > +
> > +impl Drop for RustMiscDevice {
> > +    fn drop(&mut self) {
> > +        pr_info!("Exiting the Rust Misc Device Sample\n");
> > +    }
> > +}
> > --
> > 2.47.0.338.g60cca15819-goog
> 
> 
> Benoît du Garreau

-- 
Lee Jones [李琼斯]

Re: [PATCH 1/2] samples: rust: Provide example using the new Rust MiscDevice abstraction

[Lee Jones]

On Thu, 05 Dec 2024, Lee Jones wrote:

> On Thu, 05 Dec 2024, Benoît du Garreau wrote:
> 
> > On Wed, 4 Dec 2024 17:46:24 +0000 Lee Jones <lee@kernel.org> wrote:
> > 
> > > This sample driver demonstrates the following basic operations:
> > >
> > > * Register a Misc Device
> > > * Create /dev/rust-misc-device
> > > * Open the aforementioned character device
> > > * Operate on the character device via a simple ioctl()
> > > * Close the character device
> > >
> > > Signed-off-by: Lee Jones <lee@kernel.org>
> > > ---
> > >  samples/rust/Kconfig             | 10 ++++
> > >  samples/rust/Makefile            |  1 +
> > >  samples/rust/rust_misc_device.rs | 84 ++++++++++++++++++++++++++++++++
> > >  3 files changed, 95 insertions(+)
> > >  create mode 100644 samples/rust/rust_misc_device.rs
> > >
> > > diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> > > index b0f74a81c8f9..df384e679901 100644
> > > --- a/samples/rust/Kconfig
> > > +++ b/samples/rust/Kconfig
> > > @@ -20,6 +20,16 @@ config SAMPLE_RUST_MINIMAL
> > >
> > >  	  If unsure, say N.
> > >
> > > +config SAMPLE_RUST_MISC_DEVICE
> > > +	tristate "Misc device"
> > > +	help
> > > +	  This option builds the Rust misc device.
> > > +
> > > +	  To compile this as a module, choose M here:
> > > +	  the module will be called rust_misc_device.
> > > +
> > > +	  If unsure, say N.
> > > +
> > >  config SAMPLE_RUST_PRINT
> > >  	tristate "Printing macros"
> > >  	help
> > > diff --git a/samples/rust/Makefile b/samples/rust/Makefile
> > > index c1a5c1655395..ad4b97a98580 100644
> > > --- a/samples/rust/Makefile
> > > +++ b/samples/rust/Makefile
> > > @@ -2,6 +2,7 @@
> > >  ccflags-y += -I$(src)				# needed for trace events
> > >
> > >  obj-$(CONFIG_SAMPLE_RUST_MINIMAL)		+= rust_minimal.o
> > > +obj-$(CONFIG_SAMPLE_RUST_MISC_DEVICE)		+= rust_misc_device.o
> > >  obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
> > >
> > >  rust_print-y := rust_print_main.o rust_print_events.o
> > > diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> > > new file mode 100644
> > > index 000000000000..5f1b69569ef7
> > > --- /dev/null
> > > +++ b/samples/rust/rust_misc_device.rs
> > > @@ -0,0 +1,84 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +
> > > +//! Rust misc device sample.
> > > +
> > > +use kernel::{
> > > +    c_str,
> > > +    ioctl::_IO,
> > > +    miscdevice::{MiscDevice, MiscDeviceOptions, MiscDeviceRegistration},
> > > +    prelude::*,
> > > +};
> > > +
> > > +const RUST_MISC_DEV_HELLO: u32 = _IO('R' as u32, 9);
> > > +
> > > +module! {
> > > +    type: RustMiscDeviceModule,
> > > +    name: "rust_misc_device",
> > > +    author: "Lee Jones",
> > > +    description: "Rust misc device sample",
> > > +    license: "GPL",
> > > +}
> > > +
> > > +struct RustMiscDeviceModule {
> > > +    _miscdev: Pin<KBox<MiscDeviceRegistration<RustMiscDevice>>>,
> > > +}
> > > +
> > > +impl kernel::Module for RustMiscDeviceModule {
> > 
> > This could probably use `kernel::InPlaceModule` and avoid allocating
> > the registration.
> 
> Is there a way to do this and keep the print?

Seeing as this is the concept/API demonstrator, I think what I'd sooner
do, is demonstrate some actual initialisation.  Maybe with a head-nod to
`kernel::InPlaceModule` in a comment.

-- 
Lee Jones [李琼斯]