netdev - Re: [PATCH net-next v6 1/5] 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: <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
>> +    }
> [...]
> 
>