netdev - Re: [PATCH net-next v3 1/3] rust: core abstractions for network PHY drivers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANiq72nBSyQw+vFayPco5b_-DDAKNqmhE7xiXSVbg920_ttAeQ@mail.gmail.com>
Date: Mon, 9 Oct 2023 14:59:19 +0200
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, andrew@...n.ch, 
	greg@...ah.com, tmgross@...ch.edu, Wedson Almeida Filho <wedsonaf@...il.com>
Subject: Re: [PATCH net-next v3 1/3] rust: core abstractions for network PHY drivers

Hi Tomonori,

A few nits I noticed. Please note that this is not really a full
review, and that I recommend that other people like Wedson should take
a look again and OK these abstractions before this is merged.

On Mon, Oct 9, 2023 at 3:41 AM FUJITA Tomonori
<fujita.tomonori@...il.com> wrote:
>
> +config RUST_PHYLIB_BINDINGS

This should be called ABSTRACTIONS. Please see:

    https://docs.kernel.org/rust/general-information.html#abstractions-vs-bindings

Also, could this symbol go elsewhere?

> +        bool "PHYLIB bindings support"

Ditto.

> +          a wrapper around the C phlib core.

Typo.

> +               --rustified-enum phy_state\

As I said elsewhere, we should avoid `--rustified-enum` due tot he
risk of UB unless we are explicit on the assumptions we are placing on
the C side.

> +#![feature(const_maybe_uninit_zeroed)]

The patch message should justify this addition and warn about it.

> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> new file mode 100644
> index 000000000000..f31983bf0460
> --- /dev/null
> +++ b/rust/kernel/net/phy.rs
> @@ -0,0 +1,733 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@...il.com>

Newline missing.

> +    /// Full-duplex mode

Please use the style of the rest of the Rust comments.

> +/// An instance of a PHY device.
> +/// Wraps the kernel's `struct phy_device`.

That should be separated.

> +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else

Missing Markdown around the lifetime.

> +        // FIXME: enum-cast

Please explain what needs to be fixed.

> +    /// Executes software reset the PHY via BMCR_RESET bit.

Markdown missing (multiple instances).

> +    /// Reads Link partner ability.

Why is "link" capitalized here?

> +/// Creates the kernel's `phy_driver` instance.
> +///
> +/// This is used by [`module_phy_driver`] macro to create a static array of phy_driver`.

Broken formatting? Does `rustdoc` complain about it?

> +/// The `drivers` points to an array of `struct phy_driver`, which is
> +/// registered to the kernel via `phy_drivers_register`.

Perhaps "The `drivers` field"?

> +            // SAFETY: The type invariants guarantee that self.drivers is valid.

Markdown.

> +/// Represents the kernel's `struct mdio_device_id`.
> +pub struct DeviceId {
> +    /// Corresponds to `phy_id` in `struct mdio_device_id`.
> +    pub id: u32,
> +    mask: DeviceMask,
> +}

It would be nice to explain why the field is `pub`.

> +    /// Get a mask as u32.

Markdown.

This patch could be split a bit too, but that is up to the maintainers.

> +/// Declares a kernel module for PHYs drivers.
> +///
> +/// This creates a static array of `struct phy_driver` and registers it.

"kernel's" or similar

> +/// This also corresponds to the kernel's MODULE_DEVICE_TABLE macro, which embeds the information

Markdown.

> +/// for module loading into the module binary file. Every driver needs an entry in device_table.

Markdown.

> +/// # Examples
> +///
> +/// ```ignore
> +///
> +/// use kernel::net::phy::{self, DeviceId, Driver};
> +/// use kernel::prelude::*;
> +///
> +/// kernel::module_phy_driver! {
> +///     drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
> +///     device_table: [
> +///         DeviceId::new_with_driver::<PhyAX88772A>(),
> +///         DeviceId::new_with_driver::<PhyAX88772C>(),
> +///         DeviceId::new_with_driver::<PhyAX88796B>()
> +///     ],
> +///     name: "rust_asix_phy",
> +///     author: "Rust for Linux Contributors",
> +///     description: "Rust Asix PHYs driver",
> +///     license: "GPL",
> +/// }
> +/// ```

Please add an example above with the expansion of the macro so that it
is easy to understand at a glance, see e.g. what Benno did in
`pin-init` (`rust/init*`).

Also, perhaps splitting the patches into a few would help.

Cheers,
Miguel