Re: [PATCH] rust: virtio: add virtio support

[Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>]

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