[PATCH v5 0/4] rust: miscdevice: Provide sample driver using the new MiscDevice bindings

[PATCH v5 0/4] rust: miscdevice: Provide sample driver using the new MiscDevice bindings

[Lee Jones]

It has been suggested that the driver should use dev_info() instead of
pr_info() however there is currently no scaffolding to successfully pull
a 'struct device' out from driver data post register().  This is being
worked on and we will convert this over in due course.

Lee Jones (4):
  Documentation: ioctl-number: Carve out some identifiers for use by
    sample drivers
  samples: rust: Provide example using the new Rust MiscDevice
    abstraction
  sample: rust_misc_device: Demonstrate additional get/set value
    functionality
  MAINTAINERS: Add Rust Misc Sample to MISC entry

 .../userspace-api/ioctl/ioctl-number.rst      |   1 +
 MAINTAINERS                                   |   1 +
 samples/rust/Kconfig                          |  10 ++
 samples/rust/Makefile                         |   1 +
 samples/rust/rust_misc_device.rs              | 132 ++++++++++++++++++
 5 files changed, 145 insertions(+)
 create mode 100644 samples/rust/rust_misc_device.rs

-- 
2.47.0.338.g60cca15819-goog

[PATCH v5 1/4] Documentation: ioctl-number: Carve out some identifiers for use by sample drivers

[Lee Jones]

32 IDs should be plenty (at yet, not too greedy) since a lot of sample
drivers will use their own subsystem allocations.

Sample drivers predominately reside in <KERNEL_ROOT>/samples, but there
should be no issue with in-place example drivers using them.

Signed-off-by: Lee Jones <lee@kernel.org>
---
 Documentation/userspace-api/ioctl/ioctl-number.rst | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
index 243f1f1b554a..dc4bc0cab69f 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -311,6 +311,7 @@ Code  Seq#    Include File                                           Comments
                                                                      <mailto:oe@port.de>
 'z'   10-4F  drivers/s390/crypto/zcrypt_api.h                        conflict!
 '|'   00-7F  linux/media.h
+'|'   80-9F  samples/                                                Any sample and example drivers
 0x80  00-1F  linux/fb.h
 0x81  00-1F  linux/vduse.h
 0x89  00-06  arch/x86/include/asm/sockios.h
-- 
2.47.0.338.g60cca15819-goog

[PATCH v5 2/4] 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
* Provide open call-back for the aforementioned character device
* Operate on the character device via a simple ioctl()
* Provide close call-back for 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 | 79 ++++++++++++++++++++++++++++++++
 3 files changed, 90 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..c94b56c454fb
--- /dev/null
+++ b/samples/rust/rust_misc_device.rs
@@ -0,0 +1,79 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2024 Google LLC.
+
+//! Rust misc device sample.
+
+use kernel::{
+    c_str,
+    ioctl::_IO,
+    miscdevice::{MiscDevice, MiscDeviceOptions, MiscDeviceRegistration},
+    prelude::*,
+};
+
+const RUST_MISC_DEV_HELLO: u32 = _IO('|' as u32, 0x80);
+
+module! {
+    type: RustMiscDeviceModule,
+    name: "rust_misc_device",
+    author: "Lee Jones",
+    description: "Rust misc device sample",
+    license: "GPL",
+}
+
+#[pin_data]
+struct RustMiscDeviceModule {
+    #[pin]
+    _miscdev: MiscDeviceRegistration<RustMiscDevice>,
+}
+
+impl kernel::InPlaceModule for RustMiscDeviceModule {
+    fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
+        pr_info!("Initialising Rust Misc Device Sample\n");
+
+        let options = MiscDeviceOptions {
+            name: c_str!("rust-misc-device"),
+        };
+
+        try_pin_init!(Self {
+            _miscdev <- MiscDeviceRegistration::register(options),
+        })
+    }
+}
+
+struct RustMiscDevice;
+
+#[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, 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(ENOTTY);
+            }
+        }
+
+        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 v5 3/4] 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;
}

And here is the output (manually spliced together):

USERSPACE: Opening /dev/rust-misc-device for reading and writing
KERNEL: rust_misc_device: Opening Rust Misc Device Sample
USERSPACE: Calling Hello
KERNEL: rust_misc_device: IOCTLing Rust Misc Device Sample
KERNEL: rust_misc_device: -> Hello from the Rust Misc Device
USERSPACE: Fetching initial value
KERNEL: rust_misc_device: IOCTLing Rust Misc Device Sample
KERNEL: rust_misc_device: -> Copying data to userspace (value: 0)
USERSPACE: Submitting new value (1)
KERNEL: rust_misc_device: IOCTLing Rust Misc Device Sample
KERNEL: rust_misc_device: -> Copying data from userspace (value: 1)
USERSPACE: Fetching new value
KERNEL: rust_misc_device: IOCTLing Rust Misc Device Sample
KERNEL: rust_misc_device: -> Copying data to userspace (value: 1)
USERSPACE: Attempting to call in to an non-existent IOCTL
KERNEL: rust_misc_device: IOCTLing Rust Misc Device Sample
KERNEL: rust_misc_device: -> IOCTL not recognised: 20992
USERSPACE: ioctl: Succeeded to fail - this was expected: Inappropriate ioctl for device
USERSPACE: Closing /dev/rust-misc-device
KERNEL: rust_misc_device: Exiting the Rust Misc Device Sample
USERSPACE: Success

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

diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
index c94b56c454fb..8145cf072640 100644
--- a/samples/rust/rust_misc_device.rs
+++ b/samples/rust/rust_misc_device.rs
@@ -4,14 +4,21 @@
 
 //! 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_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);
 
 module! {
     type: RustMiscDeviceModule,
@@ -41,39 +48,84 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
     }
 }
 
-struct RustMiscDevice;
+struct Inner {
+    value: i32,
+}
+
+#[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, 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");
 
+        let size = _IOC_SIZE(cmd);
+
         match cmd {
-            RUST_MISC_DEV_HELLO => pr_info!("Hello from the Rust Misc Device\n"),
+            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(ENOTTY);
             }
-        }
+        };
 
         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();
+        let value = guard.value;
+
+        // Refrain from calling write() on a locked resource
+        drop(guard);
+
+        pr_info!("-> Copying data to userspace (value: {})\n", &value);
+
+        writer.write::<i32>(&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

[PATCH v5 4/4] MAINTAINERS: Add Rust Misc Sample to MISC entry

[Lee Jones]

Signed-off-by: Lee Jones <lee@kernel.org>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 21f855fe468b..ea5f7c628235 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5328,6 +5328,7 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git
 F:	drivers/char/
 F:	drivers/misc/
 F:	include/linux/miscdevice.h
+F:	samples/rust/rust_misc_device.rs
 X:	drivers/char/agp/
 X:	drivers/char/hw_random/
 X:	drivers/char/ipmi/
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH v5 0/4] rust: miscdevice: Provide sample driver using the new MiscDevice bindings

[Danilo Krummrich]

On Fri, Dec 06, 2024 at 12:42:11PM +0000, Lee Jones wrote:
> It has been suggested that the driver should use dev_info() instead of
> pr_info() however there is currently no scaffolding to successfully pull
> a 'struct device' out from driver data post register().  This is being
> worked on and we will convert this over in due course.

I think you're going too fast with this series.

Please address the comments you receive before sending out new versions.

Also, please document the changes you have made from one version to the next,
otherwise it's gonna be very hard to review this.

Thanks,
Danilo

> 
> Lee Jones (4):
>   Documentation: ioctl-number: Carve out some identifiers for use by
>     sample drivers
>   samples: rust: Provide example using the new Rust MiscDevice
>     abstraction
>   sample: rust_misc_device: Demonstrate additional get/set value
>     functionality
>   MAINTAINERS: Add Rust Misc Sample to MISC entry
> 
>  .../userspace-api/ioctl/ioctl-number.rst      |   1 +
>  MAINTAINERS                                   |   1 +
>  samples/rust/Kconfig                          |  10 ++
>  samples/rust/Makefile                         |   1 +
>  samples/rust/rust_misc_device.rs              | 132 ++++++++++++++++++
>  5 files changed, 145 insertions(+)
>  create mode 100644 samples/rust/rust_misc_device.rs
> 
> -- 
> 2.47.0.338.g60cca15819-goog
> 
> 

Re: [PATCH v5 0/4] rust: miscdevice: Provide sample driver using the new MiscDevice bindings

[Lee Jones]

On Fri, 06 Dec 2024, Danilo Krummrich wrote:

> On Fri, Dec 06, 2024 at 12:42:11PM +0000, Lee Jones wrote:
> > It has been suggested that the driver should use dev_info() instead of
> > pr_info() however there is currently no scaffolding to successfully pull
> > a 'struct device' out from driver data post register().  This is being
> > worked on and we will convert this over in due course.
> 
> I think you're going too fast with this series.
> 
> Please address the comments you receive before sending out new versions.
> 
> Also, please document the changes you have made from one version to the next,
> otherwise it's gonna be very hard to review this.

I can add a change log.

What comments were missed?

-- 
Lee Jones [李琼斯]

Re: [PATCH v5 3/4] sample: rust_misc_device: Demonstrate additional get/set value functionality

[Greg KH]

On Fri, Dec 06, 2024 at 12:42:14PM +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.

nit, subject line should have "samples" not "sample" :)

> +    fn get_value(&self, mut writer: UserSliceWriter) -> Result<isize> {
> +        let guard = self.inner.lock();
> +        let value = guard.value;
> +
> +        // Refrain from calling write() on a locked resource
> +        drop(guard);
> +
> +        pr_info!("-> Copying data to userspace (value: {})\n", &value);
> +
> +        writer.write::<i32>(&value)?;
> +        Ok(0)
> +    }

I don't understand why you have to drop the mutex before calling
pr_info() and write (i.e. copy_to_user())?  It's a mutex, not a
spinlock, so you can hold it over that potentially-sleeping call, right?
Or is there some other reason why here?

thanks,

greg k-h

Re: [PATCH v5 3/4] sample: rust_misc_device: Demonstrate additional get/set value functionality

[Lee Jones]

On Fri, 06 Dec 2024, Greg KH wrote:

> On Fri, Dec 06, 2024 at 12:42:14PM +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.
> 
> nit, subject line should have "samples" not "sample" :)

Ack.

> > +    fn get_value(&self, mut writer: UserSliceWriter) -> Result<isize> {
> > +        let guard = self.inner.lock();
> > +        let value = guard.value;
> > +
> > +        // Refrain from calling write() on a locked resource
> > +        drop(guard);
> > +
> > +        pr_info!("-> Copying data to userspace (value: {})\n", &value);
> > +
> > +        writer.write::<i32>(&value)?;
> > +        Ok(0)
> > +    }
> 
> I don't understand why you have to drop the mutex before calling
> pr_info() and write (i.e. copy_to_user())?  It's a mutex, not a
> spinlock, so you can hold it over that potentially-sleeping call, right?
> Or is there some other reason why here?

This was a request from Alice to demonstrate how to unlock a mutex.

-- 
Lee Jones [李琼斯]

Re: [PATCH v5 0/4] rust: miscdevice: Provide sample driver using the new MiscDevice bindings

[Danilo Krummrich]

On Fri, Dec 06, 2024 at 12:54:30PM +0000, Lee Jones wrote:
> On Fri, 06 Dec 2024, Danilo Krummrich wrote:
> 
> > On Fri, Dec 06, 2024 at 12:42:11PM +0000, Lee Jones wrote:
> > > It has been suggested that the driver should use dev_info() instead of
> > > pr_info() however there is currently no scaffolding to successfully pull
> > > a 'struct device' out from driver data post register().  This is being
> > > worked on and we will convert this over in due course.
> > 
> > I think you're going too fast with this series.
> > 
> > Please address the comments you receive before sending out new versions.
> > 
> > Also, please document the changes you have made from one version to the next,
> > otherwise it's gonna be very hard to review this.
> 
> I can add a change log.
> 
> What comments were missed?

I think MiscDevice should ideally use the generic `Registration` type from [1].

I see that you use `InPlaceModule` now, which is fine. But since this is just a
sample, we could probably afford to wait until the generic type lands.

Also, there was a comment about how we can make use of the `dev_*` macros.

I really think we should fix those before we land a sample driver. It's gonna
be hard to explain people later on that they shouldn't do what the example
does...

[1] https://lore.kernel.org/rust-for-linux/20241205141533.111830-3-dakr@kernel.org/

> 
> -- 
> Lee Jones [李琼斯]

Re: [PATCH v5 3/4] sample: rust_misc_device: Demonstrate additional get/set value functionality

[Lee Jones]

On Fri, 06 Dec 2024, Lee Jones wrote:

> On Fri, 06 Dec 2024, Greg KH wrote:
> 
> > On Fri, Dec 06, 2024 at 12:42:14PM +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.
> > 
> > nit, subject line should have "samples" not "sample" :)
> 
> Ack.
> 
> > > +    fn get_value(&self, mut writer: UserSliceWriter) -> Result<isize> {
> > > +        let guard = self.inner.lock();
> > > +        let value = guard.value;
> > > +
> > > +        // Refrain from calling write() on a locked resource
> > > +        drop(guard);
> > > +
> > > +        pr_info!("-> Copying data to userspace (value: {})\n", &value);
> > > +
> > > +        writer.write::<i32>(&value)?;
> > > +        Ok(0)
> > > +    }
> > 
> > I don't understand why you have to drop the mutex before calling
> > pr_info() and write (i.e. copy_to_user())?  It's a mutex, not a
> > spinlock, so you can hold it over that potentially-sleeping call, right?
> > Or is there some other reason why here?
> 
> This was a request from Alice to demonstrate how to unlock a mutex.

It's common practice to apply guards only around the protected value.

Why would this be different?

-- 
Lee Jones [李琼斯]

Re: [PATCH v5 3/4] sample: rust_misc_device: Demonstrate additional get/set value functionality

[Greg KH]

On Fri, Dec 06, 2024 at 01:06:30PM +0000, Lee Jones wrote:
> On Fri, 06 Dec 2024, Lee Jones wrote:
> > On Fri, 06 Dec 2024, Greg KH wrote:
> > > On Fri, Dec 06, 2024 at 12:42:14PM +0000, Lee Jones wrote:
> > > > +    fn get_value(&self, mut writer: UserSliceWriter) -> Result<isize> {
> > > > +        let guard = self.inner.lock();
> > > > +        let value = guard.value;
> > > > +
> > > > +        // Refrain from calling write() on a locked resource
> > > > +        drop(guard);
> > > > +
> > > > +        pr_info!("-> Copying data to userspace (value: {})\n", &value);
> > > > +
> > > > +        writer.write::<i32>(&value)?;
> > > > +        Ok(0)
> > > > +    }
> > > 
> > > I don't understand why you have to drop the mutex before calling
> > > pr_info() and write (i.e. copy_to_user())?  It's a mutex, not a
> > > spinlock, so you can hold it over that potentially-sleeping call, right?
> > > Or is there some other reason why here?
> > 
> > This was a request from Alice to demonstrate how to unlock a mutex.
> 
> It's common practice to apply guards only around the protected value.
> 
> Why would this be different?

It isn't, it's just that you are implying that the guard has to be
dropped because of the call to write(), which is confusing.  It's only
"needed" because you want to guard a single cpu instruction that is
guaranteed atomic by the processor :)

As this is an example driver, documentation is essential, so maybe the
comment should be:
	// Drop the mutex as we can now use our local copy
or something like that.

thanks,

greg k-h

Re: [PATCH v5 0/4] rust: miscdevice: Provide sample driver using the new MiscDevice bindings

[Lee Jones]

On Fri, 06 Dec 2024, Danilo Krummrich wrote:

> On Fri, Dec 06, 2024 at 12:54:30PM +0000, Lee Jones wrote:
> > On Fri, 06 Dec 2024, Danilo Krummrich wrote:
> > 
> > > On Fri, Dec 06, 2024 at 12:42:11PM +0000, Lee Jones wrote:
> > > > It has been suggested that the driver should use dev_info() instead of
> > > > pr_info() however there is currently no scaffolding to successfully pull
> > > > a 'struct device' out from driver data post register().  This is being
> > > > worked on and we will convert this over in due course.
> > > 
> > > I think you're going too fast with this series.
> > > 
> > > Please address the comments you receive before sending out new versions.
> > > 
> > > Also, please document the changes you have made from one version to the next,
> > > otherwise it's gonna be very hard to review this.
> > 
> > I can add a change log.
> > 
> > What comments were missed?
> 
> I think MiscDevice should ideally use the generic `Registration` type from [1].

How can an in-tree driver use out-of-tree functionality?

> I see that you use `InPlaceModule` now, which is fine. But since this is just a
> sample, we could probably afford to wait until the generic type lands.
> 
> Also, there was a comment about how we can make use of the `dev_*` macros.
> 
> I really think we should fix those before we land a sample driver. It's gonna
> be hard to explain people later on that they shouldn't do what the example
> does...

We're authoring the sample based on what is available at the moment.

There will always be something better / more convenient coming down the
pipe.  We don't usually put off contributors pending acceptance of
out-of-tree functionality, sample or otherwise.

As I've already mentioned, I'd be _more than_ happy to keep improving
this over time as new and improved helpers / infra. arrives.

> [1] https://lore.kernel.org/rust-for-linux/20241205141533.111830-3-dakr@kernel.org/

-- 
Lee Jones [李琼斯]

Re: [PATCH v5 3/4] sample: rust_misc_device: Demonstrate additional get/set value functionality

[Lee Jones]

On Fri, 06 Dec 2024, Greg KH wrote:

> On Fri, Dec 06, 2024 at 01:06:30PM +0000, Lee Jones wrote:
> > On Fri, 06 Dec 2024, Lee Jones wrote:
> > > On Fri, 06 Dec 2024, Greg KH wrote:
> > > > On Fri, Dec 06, 2024 at 12:42:14PM +0000, Lee Jones wrote:
> > > > > +    fn get_value(&self, mut writer: UserSliceWriter) -> Result<isize> {
> > > > > +        let guard = self.inner.lock();
> > > > > +        let value = guard.value;
> > > > > +
> > > > > +        // Refrain from calling write() on a locked resource
> > > > > +        drop(guard);
> > > > > +
> > > > > +        pr_info!("-> Copying data to userspace (value: {})\n", &value);
> > > > > +
> > > > > +        writer.write::<i32>(&value)?;
> > > > > +        Ok(0)
> > > > > +    }
> > > > 
> > > > I don't understand why you have to drop the mutex before calling
> > > > pr_info() and write (i.e. copy_to_user())?  It's a mutex, not a
> > > > spinlock, so you can hold it over that potentially-sleeping call, right?
> > > > Or is there some other reason why here?
> > > 
> > > This was a request from Alice to demonstrate how to unlock a mutex.
> > 
> > It's common practice to apply guards only around the protected value.
> > 
> > Why would this be different?
> 
> It isn't, it's just that you are implying that the guard has to be
> dropped because of the call to write(), which is confusing.  It's only
> "needed" because you want to guard a single cpu instruction that is
> guaranteed atomic by the processor :)
> 
> As this is an example driver, documentation is essential, so maybe the
> comment should be:
> 	// Drop the mutex as we can now use our local copy
> or something like that.

Sounds reasonable.

I've ran out of time this week.  I'll take another peek next week.

-- 
Lee Jones [李琼斯]

Re: [PATCH v5 0/4] rust: miscdevice: Provide sample driver using the new MiscDevice bindings

[Danilo Krummrich]

On Fri, Dec 06, 2024 at 01:14:45PM +0000, Lee Jones wrote:
> On Fri, 06 Dec 2024, Danilo Krummrich wrote:
> 
> > On Fri, Dec 06, 2024 at 12:54:30PM +0000, Lee Jones wrote:
> > > On Fri, 06 Dec 2024, Danilo Krummrich wrote:
> > > 
> > > > On Fri, Dec 06, 2024 at 12:42:11PM +0000, Lee Jones wrote:
> > > > > It has been suggested that the driver should use dev_info() instead of
> > > > > pr_info() however there is currently no scaffolding to successfully pull
> > > > > a 'struct device' out from driver data post register().  This is being
> > > > > worked on and we will convert this over in due course.
> > > > 
> > > > I think you're going too fast with this series.
> > > > 
> > > > Please address the comments you receive before sending out new versions.
> > > > 
> > > > Also, please document the changes you have made from one version to the next,
> > > > otherwise it's gonna be very hard to review this.
> > > 
> > > I can add a change log.
> > > 
> > > What comments were missed?
> > 
> > I think MiscDevice should ideally use the generic `Registration` type from [1].
> 
> How can an in-tree driver use out-of-tree functionality?

AFAICT, this sample module is in the exact same stage as the generic device / driver
infrastructure.

Both are on the mailing list in discussion for inclusion into the kernel.
Labeling one as in-tree and the other one as out-of-tree is clearly misleading.

I'm just saying it would be good to align this. If the sample driver is time
critical, I have no problem if you go ahead with the current
`MiscDeviceRegistration` and `InPlaceModule`, but again, why not align it from
the get-go?

> 
> > I see that you use `InPlaceModule` now, which is fine. But since this is just a
> > sample, we could probably afford to wait until the generic type lands.
> > 
> > Also, there was a comment about how we can make use of the `dev_*` macros.
> > 
> > I really think we should fix those before we land a sample driver. It's gonna
> > be hard to explain people later on that they shouldn't do what the example
> > does...
> 
> We're authoring the sample based on what is available at the moment.

Well, for this I have to disagree, not being able to use the `dev_*` macros is
simply meaning that the abstraction is incomplete (in this aspect).

I don't see the need to land a sample driver that tells the user to do the wrong
thing, i.e. use the `pr_*` macros.

As Alice mentioned, you can get the miscdevice pointer from the file private
data in open() and then make it accessible in the other fops hooks. If we go for
this solution it will change the callbacks of `MiscDevice` and maybe even some
other architectural aspects.

This needs to be addressed first.

> 
> There will always be something better / more convenient coming down the
> pipe.  We don't usually put off contributors pending acceptance of
> out-of-tree functionality, sample or otherwise.

No one asks for this here. But if the example reveals shortcomings, we shouldn't
promote them as example.

> 
> As I've already mentioned, I'd be _more than_ happy to keep improving
> this over time as new and improved helpers / infra. arrives.
> 
> > [1] https://lore.kernel.org/rust-for-linux/20241205141533.111830-3-dakr@kernel.org/
> 
> -- 
> Lee Jones [李琼斯]

Re: [PATCH v5 0/4] rust: miscdevice: Provide sample driver using the new MiscDevice bindings

[Lee Jones]

On Fri, 06 Dec 2024, Danilo Krummrich wrote:

> On Fri, Dec 06, 2024 at 01:14:45PM +0000, Lee Jones wrote:
> > On Fri, 06 Dec 2024, Danilo Krummrich wrote:
> > 
> > > On Fri, Dec 06, 2024 at 12:54:30PM +0000, Lee Jones wrote:
> > > > On Fri, 06 Dec 2024, Danilo Krummrich wrote:
> > > > 
> > > > > On Fri, Dec 06, 2024 at 12:42:11PM +0000, Lee Jones wrote:
> > > > > > It has been suggested that the driver should use dev_info() instead of
> > > > > > pr_info() however there is currently no scaffolding to successfully pull
> > > > > > a 'struct device' out from driver data post register().  This is being
> > > > > > worked on and we will convert this over in due course.
> > > > > 
> > > > > I think you're going too fast with this series.
> > > > > 
> > > > > Please address the comments you receive before sending out new versions.
> > > > > 
> > > > > Also, please document the changes you have made from one version to the next,
> > > > > otherwise it's gonna be very hard to review this.
> > > > 
> > > > I can add a change log.
> > > > 
> > > > What comments were missed?
> > > 
> > > I think MiscDevice should ideally use the generic `Registration` type from [1].
> > 
> > How can an in-tree driver use out-of-tree functionality?
> 
> AFAICT, this sample module is in the exact same stage as the generic device / driver
> infrastructure.
> 
> Both are on the mailing list in discussion for inclusion into the kernel.
> Labeling one as in-tree and the other one as out-of-tree is clearly misleading.

If I was saying that, I'd agree with you.

I was asking how MiscDevice (in-tree) could use Registration (out-of-free).

> I'm just saying it would be good to align this. If the sample driver is time
> critical, I have no problem if you go ahead with the current
> `MiscDeviceRegistration` and `InPlaceModule`, but again, why not align it from
> the get-go?

Because it's not available yet. :)

> > > I see that you use `InPlaceModule` now, which is fine. But since this is just a
> > > sample, we could probably afford to wait until the generic type lands.
> > > 
> > > Also, there was a comment about how we can make use of the `dev_*` macros.
> > > 
> > > I really think we should fix those before we land a sample driver. It's gonna
> > > be hard to explain people later on that they shouldn't do what the example
> > > does...
> > 
> > We're authoring the sample based on what is available at the moment.
> 
> Well, for this I have to disagree, not being able to use the `dev_*` macros is
> simply meaning that the abstraction is incomplete (in this aspect).
> 
> I don't see the need to land a sample driver that tells the user to do the wrong
> thing, i.e. use the `pr_*` macros.
> 
> As Alice mentioned, you can get the miscdevice pointer from the file private
> data in open() and then make it accessible in the other fops hooks. If we go for
> this solution it will change the callbacks of `MiscDevice` and maybe even some
> other architectural aspects.
> 
> This needs to be addressed first.

The issue about ever growing dependencies _can_ be that authors have
other priorities and are slow to turn things around, which may end up
with nothing being accepted and contributors getting frustrated.

However, taking into consideration how swift Alice is with these things,
I'd be happy to wait for this part if people are insistent.

> > There will always be something better / more convenient coming down the
> > pipe.  We don't usually put off contributors pending acceptance of
> > out-of-tree functionality, sample or otherwise.
> 
> No one asks for this here. But if the example reveals shortcomings, we shouldn't
> promote them as example.

IMHO it's reasonable for the sample to represent the current status of
the frameworks in use.  As advancements / adaptions are introduced we
can use them to continually improve the example.

-- 
Lee Jones [李琼斯]

Re: [PATCH v5 0/4] rust: miscdevice: Provide sample driver using the new MiscDevice bindings

[Danilo Krummrich]

On Fri, Dec 06, 2024 at 04:49:18PM +0000, Lee Jones wrote:
> On Fri, 06 Dec 2024, Danilo Krummrich wrote:
> > > > Also, there was a comment about how we can make use of the `dev_*` macros.
> > > > 
> > > > I really think we should fix those before we land a sample driver. It's gonna
> > > > be hard to explain people later on that they shouldn't do what the example
> > > > does...
> > > 
> > > We're authoring the sample based on what is available at the moment.
> > 
> > Well, for this I have to disagree, not being able to use the `dev_*` macros is
> > simply meaning that the abstraction is incomplete (in this aspect).
> > 
> > I don't see the need to land a sample driver that tells the user to do the wrong
> > thing, i.e. use the `pr_*` macros.
> > 
> > As Alice mentioned, you can get the miscdevice pointer from the file private
> > data in open() and then make it accessible in the other fops hooks. If we go for
> > this solution it will change the callbacks of `MiscDevice` and maybe even some
> > other architectural aspects.
> > 
> > This needs to be addressed first.
> 
> The issue about ever growing dependencies _can_ be that authors have
> other priorities and are slow to turn things around, which may end up
> with nothing being accepted and contributors getting frustrated.

I would share your argumentation if

1) we'd talk about a real driver, where people are actually waiting for,
2) it'd be about a new feature, performance improvement, etc.

What we have here is different:

You wrote a sample implementation for a new and just landed abstraction that
reveals a shortcoming. (Which is great, because it means the sample already
served an important purpose.)

IMHO, the consequence should not be to merge the sample as is anyways, because
another purpose of the sample implementation is to tell people "look, this is
exactly how it should look like, please do it the same way".

Instead, we should fix the shortcoming, adjust the sample implementation and
merge it then.

Just to make it clear, for a real driver I think it would be reasonable to just
go ahead, but for a sample that should educate, we should fix things first.

> 
> However, taking into consideration how swift Alice is with these things,
> I'd be happy to wait for this part if people are insistent.
> 
> > > There will always be something better / more convenient coming down the
> > > pipe.  We don't usually put off contributors pending acceptance of
> > > out-of-tree functionality, sample or otherwise.
> > 
> > No one asks for this here. But if the example reveals shortcomings, we shouldn't
> > promote them as example.
> 
> IMHO it's reasonable for the sample to represent the current status of
> the frameworks in use.  As advancements / adaptions are introduced we
> can use them to continually improve the example.
> 
> -- 
> Lee Jones [李琼斯]

Re: [PATCH v5 0/4] rust: miscdevice: Provide sample driver using the new MiscDevice bindings

[Lee Jones]

On Fri, 06 Dec 2024, Danilo Krummrich wrote:

> On Fri, Dec 06, 2024 at 04:49:18PM +0000, Lee Jones wrote:
> > On Fri, 06 Dec 2024, Danilo Krummrich wrote:
> > > > > Also, there was a comment about how we can make use of the `dev_*` macros.
> > > > > 
> > > > > I really think we should fix those before we land a sample driver. It's gonna
> > > > > be hard to explain people later on that they shouldn't do what the example
> > > > > does...
> > > > 
> > > > We're authoring the sample based on what is available at the moment.
> > > 
> > > Well, for this I have to disagree, not being able to use the `dev_*` macros is
> > > simply meaning that the abstraction is incomplete (in this aspect).
> > > 
> > > I don't see the need to land a sample driver that tells the user to do the wrong
> > > thing, i.e. use the `pr_*` macros.
> > > 
> > > As Alice mentioned, you can get the miscdevice pointer from the file private
> > > data in open() and then make it accessible in the other fops hooks. If we go for
> > > this solution it will change the callbacks of `MiscDevice` and maybe even some
> > > other architectural aspects.
> > > 
> > > This needs to be addressed first.
> > 
> > The issue about ever growing dependencies _can_ be that authors have
> > other priorities and are slow to turn things around, which may end up
> > with nothing being accepted and contributors getting frustrated.
> 
> I would share your argumentation if
> 
> 1) we'd talk about a real driver, where people are actually waiting for,
> 2) it'd be about a new feature, performance improvement, etc.
> 
> What we have here is different:
> 
> You wrote a sample implementation for a new and just landed abstraction that
> reveals a shortcoming. (Which is great, because it means the sample already
> served an important purpose.)
> 
> IMHO, the consequence should not be to merge the sample as is anyways, because
> another purpose of the sample implementation is to tell people "look, this is
> exactly how it should look like, please do it the same way".
> 
> Instead, we should fix the shortcoming, adjust the sample implementation and
> merge it then.
> 
> Just to make it clear, for a real driver I think it would be reasonable to just
> go ahead, but for a sample that should educate, we should fix things first.

Provided that we stay within certain tolerances, I don't see any of
what you've said as particularly unreasonable.  I'll have an out-of-band
chat with Alice on Monday with a view to conjuring up a game plan.

-- 
Lee Jones [李琼斯]