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

[Greg KH <gregkh@linuxfoundation.org>]

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