Messages in this thread | | | From | Miguel Ojeda <> | Date | Tue, 7 Mar 2023 19:08:55 +0100 | Subject | Re: [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
| |