| 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
| ||
|
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
Powered by blists - more mailing lists