lkml.org 
[lkml]   [2023]   [Mar]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH] rust: virtio: add virtio support
    On Tue, Mar 7, 2023 at 2:04 PM Daniel Almeida
    <daniel.almeida@collabora.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

    \
     
     \ /
      Last update: 2023-03-27 00:47    [W:6.923 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site