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: <46b4ea56-1b66-4a8f-8c30-ecea895638b2@proton.me> Date: Wed, 25 Oct 2023 07:24:00 +0000 From: Benno Lossin <benno.lossin@...ton.me> To: FUJITA Tomonori <fujita.tomonori@...il.com> Cc: 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 25.10.23 03:10, FUJITA Tomonori wrote: > 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. Sorry, my bad, `rustfmt` does not format comments, so you have to do them manually. > What's the limit? The limit is 100 characters. >> 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. That is one way, another would be to do: /// PHY state machine states. /// /// Corresponds to the kernel's [`enum phy_state`]. /// /// Some of PHY drivers access to the state of PHY's software state machine. /// /// [`enum phy_state`]: ../../../../../networking/kapi.html#c.phy_state But as I noted before, then people who only build the rustdoc will not be able to view it. I personally would prefer to have the correct link offline, but do not know about others. >>> + /// Gets the current link state. It returns true if the link is up. I just noticed this as well, here also please split the line. >>> + 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. I would prefer this, since the function name itself already is pretty self-explanatory. If someone really wants to understand it, they probably have to read the source code. -- Cheers, Benno
Powered by blists - more mailing lists