Message-ID: <aJRZ3NUm2xp2H4iX@sidongui-MacBookPro.local>
Date: Thu, 7 Aug 2025 16:46:36 +0900
From: Sidong Yang <sidong.yang@...iosa.ai>
To: Daniel Almeida <daniel.almeida@...labora.com>
Cc: Caleb Sander Mateos <csander@...estorage.com>,
	Benno Lossin <lossin@...nel.org>, Miguel Ojeda <ojeda@...nel.org>,
	Arnd Bergmann <arnd@...db.de>, Jens Axboe <axboe@...nel.dk>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
	io-uring@...r.kernel.org
Subject: Re: [RFC PATCH v2 3/4] rust: miscdevice: add uring_cmd() for
 MiscDevice trait

On Fri, Aug 01, 2025 at 11:04:36AM -0300, Daniel Almeida wrote:
> Hi Sidong,
> 
> > On 27 Jul 2025, at 12:03, Sidong Yang <sidong.yang@...iosa.ai> wrote:
> > 
> > This patch adds uring_cmd() function for MiscDevice trait and its
> > callback implementation. It uses IoUringCmd that io_uring_cmd rust
> > abstraction.
> 
> I can´t parse this.

Okay, I'll fix this.

> 
> > 
> > Signed-off-by: Sidong Yang <sidong.yang@...iosa.ai>
> > ---
> > rust/kernel/miscdevice.rs | 41 +++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 41 insertions(+)
> > 
> > diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> > index 288f40e79906..54be866ea7ff 100644
> > --- a/rust/kernel/miscdevice.rs
> > +++ b/rust/kernel/miscdevice.rs
> > @@ -14,6 +14,7 @@
> >     error::{to_result, Error, Result, VTABLE_DEFAULT_ERROR},
> >     ffi::{c_int, c_long, c_uint, c_ulong},
> >     fs::File,
> > +    io_uring::IoUringCmd,
> >     mm::virt::VmaNew,
> >     prelude::*,
> >     seq_file::SeqFile,
> > @@ -175,6 +176,19 @@ fn show_fdinfo(
> >     ) {
> >         build_error!(VTABLE_DEFAULT_ERROR)
> >     }
> > +
> > +    /// Handler for uring_cmd.
> > +    ///
> > +    /// This function is invoked when userspace process submits the uring_cmd op
> > +    /// on io_uring submission queue. The `io_uring_cmd` would be used for get
> > +    /// arguments cmd_op, sqe, cmd_data.
> 
> Please improve this. I don´t think that anyone reading this can really get
> a good grasp on what this function does.
> 
> What does `issue_flags` do?
> 

Agreed, Let me revise this comments to make them easier to understand.

issue_flags includes flags options for io_uring. it's defined as `io_uring_cmd_flags`
in "include/linux/io_uring_types.h".

> > +    fn uring_cmd(
> > +        _device: <Self::Ptr as ForeignOwnable>::Borrowed<'_>,
> > +        _io_uring_cmd: Pin<&mut IoUringCmd>,
> > +        _issue_flags: u32,
> > +    ) -> Result<i32> {
> > +        build_error!(VTABLE_DEFAULT_ERROR)
> > +    }
> > }
> > 
> > /// A vtable for the file operations of a Rust miscdevice.
> > @@ -332,6 +346,28 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
> >         T::show_fdinfo(device, m, file);
> >     }
> > 
> > +    /// # Safety
> > +    ///
> > +    /// `ioucmd` is not null and points to a valid `bindings::io_uring_cmd`.
> 
> Please rewrite this as "the caller must ensure that  `ioucmd` points to a
> valid `bindings::io_uring_cmd`" or some variation thereof.

Okay, Thanks.
> 
> > +    unsafe extern "C" fn uring_cmd(
> > +        ioucmd: *mut bindings::io_uring_cmd,
> > +        issue_flags: ffi::c_uint,
> > +    ) -> ffi::c_int {
> > +        // SAFETY: The file is valid for the duration of this call.
> > +        let ioucmd = unsafe { IoUringCmd::from_raw(ioucmd) };
> 
> What file?
> 
> Also, this is what you wrote for IoUringCmd::from_raw:

Sorry, it's typo. It should be rewritted.

> 
> +
> + /// Constructs a new `IoUringCmd` from a raw `io_uring_cmd`
> + ///
> + /// # Safety
> + ///
> + /// The caller must guarantee that:
> + /// - The pointer `ptr` is not null and points to a valid `bindings::io_uring_cmd`.
> + /// - The memory pointed to by `ptr` remains valid for the duration of the returned reference's lifetime `'a`.
> + /// - The memory will not be moved or freed while the returned `Pin<&mut IoUringCmd>` is alive.
> + #[inline]
> + pub unsafe fn from_raw<'a>(ptr: *mut bindings::io_uring_cmd) -> Pin<&'a mut IoUringCmd> {
> 
> Here, you have to mention how the safety requirements above are fulfilled in this call site.

Okay, Actually, I'm little confused because it seems that the unsafe code deleted in email.
Anyway I would mention it in next.

> 
> > +        let file = ioucmd.file();
> > +
> > +        // SAFETY: The file is valid for the duration of this call.
> 
> Same here.

Thanks.
> 
> > +        let private = unsafe { (*file.as_ptr()).private_data }.cast();
> 
> Perhaps this can be hidden away in an accessor?

It seems that there is no accessor for private_data in File. 

> 
> > +        // SAFETY: uring_cmd calls can borrow the private data of the file.
> > +        let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
> 
> This is ForeignOwnable::borrow():
> 
>     /// Borrows a foreign-owned object immutably.
>     ///
>     /// This method provides a way to access a foreign-owned value from Rust immutably. It provides
>     /// you with exactly the same abilities as an `&Self` when the value is Rust-owned.
>     ///
>     /// # Safety
>     ///
>     /// The provided pointer must have been returned by a previous call to [`into_foreign`], and if
>     /// the pointer is ever passed to [`from_foreign`], then that call must happen after the end of
>     /// the lifetime `'a`.
>     ///
>     /// [`into_foreign`]: Self::into_foreign
>     /// [`from_foreign`]: Self::from_foreign
>     unsafe fn borrow<'a>(ptr: *mut Self::PointedTo) -> Self::Borrowed<'a>;
> 
> You must say how the safety requirements above are fulfilled in this call site
> as well. In particular, are you sure that this is true? i.e.:
> 
> > The provided pointer must have been returned by a previous call to
> > [`into_foreign`],

Okay, I would mention that the this call fulfilled the requirements.
> 
> 
> > +
> > +        match T::uring_cmd(device, ioucmd, issue_flags) {
> > +            Ok(ret) => ret as ffi::c_int,
> > +            Err(err) => err.to_errno() as ffi::c_int,
> 
> c_int is in the prelude. Also, please have a look at error::from_result().

Thanks.


Thank you for detailed Review.
Sidong

> 
> > +        }
> > +    }
> > +
> >     const VTABLE: bindings::file_operations = bindings::file_operations {
> >         open: Some(Self::open),
> >         release: Some(Self::release),
> > @@ -354,6 +390,11 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
> >         } else {
> >             None
> >         },
> > +        uring_cmd: if T::HAS_URING_CMD {
> > +            Some(Self::uring_cmd)
> > +        } else {
> > +            None
> > +        },
> >         // SAFETY: All zeros is a valid value for `bindings::file_operations`.
> >         ..unsafe { MaybeUninit::zeroed().assume_init() }
> >     };
> > -- 
> > 2.43.0
> > 
> > 
> 
> - Daniel
> 

Powered by blists - more mailing lists