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: <20231025.101046.1989690650451477174.fujita.tomonori@gmail.com> Date: Wed, 25 Oct 2023 10:10:46 +0900 (JST) From: FUJITA Tomonori <fujita.tomonori@...il.com> To: benno.lossin@...ton.me Cc: fujita.tomonori@...il.com, netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, andrew@...n.ch, tmgross@...ch.edu, miguel.ojeda.sandonis@...il.com, wedsonaf@...il.com, greg@...ah.com Subject: Re: [PATCH net-next v6 1/5] rust: core abstractions for network PHY drivers On Tue, 24 Oct 2023 16:23:20 +0000 Benno Lossin <benno.lossin@...ton.me> wrote: > On 24.10.23 02:58, FUJITA Tomonori wrote: >> This patch adds abstractions to implement network PHY drivers; the >> driver registration and bindings for some of callback functions in >> struct phy_driver and many genphy_ functions. >> >> This feature is enabled with CONFIG_RUST_PHYLIB_ABSTRACTIONS=y. >> >> 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. It's supposed to be stable in the not so distant >> future. >> >> Link: https://github.com/rust-lang/rust/pull/116218 >> >> Signed-off-by: FUJITA Tomonori <fujita.tomonori@...il.com> > > [...] > >> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs >> new file mode 100644 >> index 000000000000..2d821c2475e1 >> --- /dev/null >> +++ b/rust/kernel/net/phy.rs >> @@ -0,0 +1,708 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> + >> +// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@...il.com> >> + >> +//! Network PHY device. >> +//! >> +//! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h). >> + >> +use crate::{ >> + bindings, >> + error::*, >> + prelude::{vtable, Pin}, >> + str::CStr, >> + types::Opaque, >> +}; >> +use core::marker::PhantomData; >> + >> +/// PHY state machine states. >> +/// >> +/// Corresponds to the kernel's [`enum phy_state`](https://docs.kernel.org/networking/kapi.html#c.phy_state). > > Please use `rustfmt`, this line is 109 characters long. Hmm, `make rustfmt` doesn't warn on my env. `make rustfmtcheck` or `make rustdoc` doesn't. What's the limit? > Also it might make sense to use a relative link, since then it also > works offline (though you have to build the C docs). /// Corresponds to the kernel's [`enum phy_state`](../../../../../networking/kapi.html#c.phy_state). 101 characters too long? Then we could write: /// PHY state machine states. /// /// Corresponds to the kernel's /// [`enum phy_state`](../../../../../networking/kapi.html#c.phy_state). /// /// Some of PHY drivers access to the state of PHY's software state machine. >> + /// Gets the current link state. It returns true if the link is up. >> + pub fn get_link(&self) -> bool { >> + const LINK_IS_UP: u32 = 1; >> + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. >> + let phydev = unsafe { *self.0.get() }; >> + phydev.link() == LINK_IS_UP >> + } > > Can we please change this name? I think Tomo is waiting for Andrew to > give his OK. All the other getter functions already follow the Rust > naming convention, so this one should as well. I think using > `is_link_up` would be ideal, since `link()` reads a bit weird in code: > > if dev.link() { > // ... > } > > vs > > if dev.is_link_up() { > // ... > } I'll go with is_link_up() >> + /// Gets the current auto-negotiation configuration. It returns true if auto-negotiation is enabled. > > Move the second sentence into a new line, it should not be part of the > one-line summary. Oops, make one-line? /// Gets the current auto-negotiation configuration and returns true if auto-negotiation is enabled. Or /// Gets the current auto-negotiation configuration. /// /// It returns true if auto-negotiation is enabled. >> + pub fn is_autoneg_enabled(&self) -> bool { >> + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. >> + let phydev = unsafe { *self.0.get() }; >> + phydev.autoneg() == bindings::AUTONEG_ENABLE >> + } >> + >> + /// Gets the current auto-negotiation state. It returns true if auto-negotiation is completed. > Same here. > > -- > Cheers, > Benno > >> + pub fn is_autoneg_completed(&self) -> bool { >> + const AUTONEG_COMPLETED: u32 = 1; >> + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. >> + let phydev = unsafe { *self.0.get() }; >> + phydev.autoneg_complete() == AUTONEG_COMPLETED >> + } > [...] > >
Powered by blists - more mailing lists