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
| ||
|
Message-Id: <20231012.125937.1346884503622296050.fujita.tomonori@gmail.com> Date: Thu, 12 Oct 2023 12:59:37 +0900 (JST) From: FUJITA Tomonori <fujita.tomonori@...il.com> To: miguel.ojeda.sandonis@...il.com Cc: fujita.tomonori@...il.com, netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, andrew@...n.ch, greg@...ah.com, tmgross@...ch.edu, wedsonaf@...il.com Subject: Re: [PATCH net-next v3 1/3] rust: core abstractions for network PHY drivers On Mon, 9 Oct 2023 14:59:19 +0200 Miguel Ojeda <miguel.ojeda.sandonis@...il.com> wrote: > 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: Fixed. > https://docs.kernel.org/rust/general-information.html#abstractions-vs-bindings > > Also, could this symbol go elsewhere? This symbol is used by the third patch. Where do you want this? >> + bool "PHYLIB bindings support" > > Ditto. Updated. >> + a wrapper around the C phlib core. > > Typo. Oops, sorry. >> +#![feature(const_maybe_uninit_zeroed)] > > The patch message should justify this addition and warn about it. I added the following to the commit log. This patch enables unstable const_maybe_uninit_zeroed feature for kernel crate to enable unsafe code to handle a constant value with uninitialized data. With the feature, the abstractions can initialize a phy_driver structure with zero easily; instead of initializing all the members by hand. >> 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. Added. >> + /// Full-duplex mode > > Please use the style of the rest of the Rust comments. I'm not sure what the style should be but something like the following? /// Represents duplex mode. pub enum DuplexMode { /// PHY is in full-duplex mode. Full, >> +/// An instance of a PHY device. >> +/// Wraps the kernel's `struct phy_device`. > > That should be separated. Added. >> + /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else > > Missing Markdown around the lifetime. Fixed. >> + // FIXME: enum-cast > > Please explain what needs to be fixed. Added. >> + /// Executes software reset the PHY via BMCR_RESET bit. > > Markdown missing (multiple instances). Can you elaborate? >> + /// Reads Link partner ability. > > Why is "link" capitalized here? Fixed. >> +/// 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? Yes, sorry about that. >> +/// The `drivers` points to an array of `struct phy_driver`, which is >> +/// registered to the kernel via `phy_drivers_register`. > > Perhaps "The `drivers` field"? I replaced this with the following comment suggested by Benno. /// All elements of the `drivers` slice are valid and currently registered /// to the kernel via `phy_drivers_register`. >> + // SAFETY: The type invariants guarantee that self.drivers is valid. > > Markdown. Fixed. >> +/// 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`. Added. >> + /// Get a mask as u32. > > Markdown. Fixed. > This patch could be split a bit too, but that is up to the maintainers. Yeah. >> +/// Declares a kernel module for PHYs drivers. >> +/// >> +/// This creates a static array of `struct phy_driver` and registers it. > > "kernel's" or similar Added. >> +/// This also corresponds to the kernel's MODULE_DEVICE_TABLE macro, which embeds the information > > Markdown. Fixed. >> +/// for module loading into the module binary file. Every driver needs an entry in device_table. > > Markdown. Fixed. >> +/// # 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*`). Added. Thanks a lot!
Powered by blists - more mailing lists