[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CANiq72nkBAfM_b50DsuoJjy3hXhBxNRK0oeU_kyQHaMnnjvGJA@mail.gmail.com>
Date: Tue, 7 Mar 2023 19:08:55 +0100
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Daniel Almeida <daniel.almeida@...labora.com>
Cc: wedsonaf@...il.com, ojeda@...nel.org,
rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
virtualization@...ts.linux-foundation.org, kernel@...labora.com
Subject: Re: [PATCH] rust: virtio: add virtio support
On Tue, Mar 7, 2023 at 2:04 PM Daniel Almeida
<daniel.almeida@...labora.com> wrote:
>
> Feel free to point me to the best practices around Rust patch
> submission, as the C stuff like checkpatch etc probably does not apply
> yet. I did take care to run clippy though.
Yeah, some nits below as if you had run a script like that :)
> #include <linux/uaccess.h>
> +#include <linux/virtio.h>
> +#include <linux/virtio_config.h>
> #include <linux/uio.h>
Please sort these.
> +#![allow(missing_docs)]
Unless there is a very good reason (which should most likely be
documented in that case), please instead document everything. We keep
lints "deny" instead of `-Fmissing_docs` just in case there is a need
somewhere, but that does not mean not documenting is OK in general.
(Also, the module-level attributes would be placed after the docs).
> +///! Virtio abstractions
> +///!
> +///! C header: [`include/linux/virtio.h`](../../../../include/media/virtio.h)
Please add a blank line after the module-level docs.
> +unsafe impl const crate::driver::RawDeviceId for DeviceId {
`unsafe impl` need `SAFETY` comments too, just like for blocks (which
Björn mentioned). I see you did one elsewhere, though.
> + // No `data` pointer.
> + fn to_rawid(&self, _offset: isize) -> Self::RawType {
I see there is no `data` pointer compared to e.g. an `amba_id`, but
what the comment means? Is it about that that is the reason for
`_offset` being unused? Or something else?
> + from_kernel_result! {
We will be going away from `from_kernel_result!` (in case you wonder
when rebasing in the future).
> + // SAFETY: `virtio_device` is guaranteed to be a valid, non-null
> + // pointer. `priv_` was set on probe().
Markdown: probe() -> `probe()`. Same elsewhere where possible to be consistent.
> + // SAFETY:
> + // - we allocated this pointer using `T::Data::into_foreign`,
> + // so it is safe to turn back into a `T::Data`.
Please start the sentences with uppercase (same elsewhere).
> + /// The table of device ids supported by the driver.
Maybe "device ids" -> "`DeviceId`s"?
Somewhere else you used an intra-doc link, which is great. Please use
them wherever possible, e.g. if you go for the type here, maybe it
works too i.e. [`DeviceId`]
> + /// A wrapper over virtqueue_add_sgs()
If you are referring to the C one, normally we note that (i.e. in
order to distinguish it from a Rust one). Also period at the end.
For instance:
/// A wrapper over the C side `virtqueue_add_sgs()`.
However, the function should instead document what it does, rather
than just refer to the C one. Same elsewhere.
> + if buf.is_null() {
> + return None;
> + } else {
> + // SAFETY: if there is a buffer token, it came from
> + // into_foreign() as called in add_sgs.
> + <T::PrivateData as ForeignOwnable>::from_foreign(buf)
> + }
No need for `else` after `return`, so you can remove the indentation
too. Also Markdown.
> + pub(crate) unsafe extern "C" fn vq_callback(vq: *mut bindings::virtqueue) {
> + // SAFETY: the caller should guarantee that vq is valid for the lifetime
> + // of Self.
Markdown and uppercase -- here and elsewhere, e.g.
// SAFETY: The caller should guarantee that `vq` is valid for the lifetime
// of `Self`.
> + device: 0, // The protocol ID.
Please place the comment on top.
> + // let virtqueue = virtio_dev.find_virtqueue::<VirtqueueCallback>(kernel::c_str!(""))?;
// ```rust
// let ...
// ```
Thanks for the patch!
Cheers,
Miguel
Powered by blists - more mailing lists